Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve IP configuration UI #22320

Merged
merged 7 commits into from
Oct 21, 2024
Merged

Improve IP configuration UI #22320

merged 7 commits into from
Oct 21, 2024

Conversation

MindFreeze
Copy link
Contributor

Proposed change

Split netmask from IP address only on the frontend for presentation.
The netmask on the IP address is confusing for users.
The goal is to make a separate netmask field (255.255.255.0 format).

image
image

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@piitaya
Copy link
Member

piitaya commented Oct 14, 2024

If we have multiple addresses, should be use multiple fields (one per address)? Otherwise we will have something like :

Address
192.168.1.2, 10.0.0.1

Netmask
255.255.255.255, 255.0.0.0

@MindFreeze
Copy link
Contributor Author

If we have multiple addresses, should be use multiple fields (one per address)? Otherwise we will have something like :

Address 192.168.1.2, 10.0.0.1

Netmask 255.255.255.255, 255.0.0.0

The thing is that addresses are still combined in the API so they can in theory have different masks. I also don't think multiple addresses are a common enough case to bother but I could be wrong.

@piitaya
Copy link
Member

piitaya commented Oct 14, 2024

The problem is that it adds a lot of complexity in the code for an uncommon use-case.

Should we create two method to transform the address format so at least the code is simpler so at least we don't need to splitting and join logic in the component?

For example :

const formatAddress = (host: string, netmask: string) => string;
const parseAddress = (fullAddress: string) => { host: string, netmask: string };

@MindFreeze
Copy link
Contributor Author

That's exactly how I started but it turned out these just replace address.split('/)[0] with parseAddress(address).host. This seemed pointless to me.

@MindFreeze
Copy link
Contributor Author

And I always need/have the address OR the mask and didn't want to convert both every time.

@MindFreeze MindFreeze marked this pull request as draft October 17, 2024 18:07
@MindFreeze
Copy link
Contributor Author

  • put IP & Netmask on 1 line with add button
  • split DNS values and add button with dropdown
  • show values when in Automatic mode and remove IP info dialog
    image

@MindFreeze MindFreeze marked this pull request as ready for review October 20, 2024 10:07
@MindFreeze MindFreeze changed the title Split netmask from IP address Improve IP configuration UI Oct 20, 2024
@agners
Copy link
Member

agners commented Oct 21, 2024

The thing is that addresses are still combined in the API so they can in theory have different masks. I also don't think multiple addresses are a common enough case to bother but I could be wrong.

Yeah I agree for IPv4 supporting a single IP address if fine, at least for the UI. We could support this through CLI if needs to be. For IPv6 this is more common, but since we use the netmask format for IPv6 that is not a problem there. In IPv6, setting a static IP is rather uncommon in general, so most people should be able to leave the setting in automatic for IPv6.

I like that new view! 🤩

As for netmask, what you can do is suggest a netmask. Even though we are in a classless world these days, the classes still remain a common default. So essentially, if an IPv4 of Class A is typed, suggest 255.0.0.0, etc. (see https://en.wikipedia.org/wiki/Classful_network#Classful_addressing_definition). Most people probably type a Class C address.

Copy link
Member

@piitaya piitaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice ! I like it 🙂 I only submitted small review comments about code style.

src/panels/config/network/supervisor-network.ts Outdated Show resolved Hide resolved
src/panels/config/network/supervisor-network.ts Outdated Show resolved Hide resolved
src/panels/config/network/supervisor-network.ts Outdated Show resolved Hide resolved
src/panels/config/network/supervisor-network.ts Outdated Show resolved Hide resolved
@MindFreeze MindFreeze merged commit 202bc64 into home-assistant:dev Oct 21, 2024
15 checks passed
@MindFreeze MindFreeze deleted the netmask branch October 21, 2024 18:30
@agners
Copy link
Member

agners commented Oct 21, 2024

Hm, does this commit means that we have a separate mask field for IPv6 with lots of ff?

a31470a

I don't think that is a good idea. Noone is doing IPv6 masks that way, it is just waay to much to write. If we don't want the implicit /, simply use a separate field for the prefix in numeric, that is what all OSes do (e.g. see Windows XP).

When it comes to network config dialog, doing what others do is really the right way to go, since that is what folks recognize and understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants