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

Migrate SSID dialog to Compose, move + rename prioritize internal #2662

Merged
merged 2 commits into from Jul 9, 2022

Conversation

jpelgrom
Copy link
Member

@jpelgrom jpelgrom commented Jul 7, 2022

Summary

This PR:

There are no changes to the functionality.

Screenshots

Light Dark
Main settings screen: removed 'Prioritize Internal' The main companion app settings screen, showing the header Connection Information with the options Home Assistant URL, Home Network WiFi SSID and Internal Connection URL, in light mode The main companion app settings screen, showing the header Connection Information with the options Home Assistant URL, Home Network WiFi SSID and Internal Connection URL, in dark mode
New SSID screen instead of dialog A screen with the title 'Home Network WiFi SSID', which shows an input and a list of SSIDs, and an option that allows choosing how to handle the URL for the webview when location is turned off, in light mode A screen with the title 'Home Network WiFi SSID', which shows an input and a list of SSIDs, and an option that allows choosing how to handle the URL for the webview when location is turned off, in dark mode
Renamed Prioritize Internal is now a dropdown The option 'When loading the dashboard and unknown if connected to Home Network WiFi' is selected and shows two choices: 'Use Home Assistant URL (recommended)' and 'Use internal connection URL (use this if you typically turn location access off', in light mode The option 'When loading the dashboard and unknown if connected to Home Network WiFi' is selected and shows two choices: 'Use Home Assistant URL (recommended)' and 'Use internal connection URL (use this if you typically turn location access off', in dark mode

Link to pull request in Documentation repository

n/a

Any other notes

A potential future improvement: move setting the internal connection URL to this screen as well, so all internal connection-related settings are in the same place and don't clutter up the main settings screen. Not including it right now to keep the PR easier to review, otherwise it would require handling permissions which will add a lot of code.

 - Replaces the SSID dialog with a new screen built with Compose (same functionality as before)
 - Move the prioritize internal setting to the new SSID screen where it makes more sense, and rename it + style it as a dropdown to prevent unnecessary changes
@dshokouhi
Copy link
Member

Do you think we should make the connection options more distinguished with radio buttons like we do in other places or is it intentional to make it look like it can't change via drop down?

@jpelgrom
Copy link
Member Author

jpelgrom commented Jul 7, 2022

The intention was to de-emphasize the setting, but not to make it look like it cannot be changed... It's a row similar to sensor settings / default Android settings.

I do think the radio buttons on other screens like the sensor update frequency are a bit much for this setting. Quick test with 'smaller' radio buttons:
image
What do you think?

@dshokouhi
Copy link
Member

Thanks for the screenshot to compare! Yea I see why you went for a drop down, its a bit much here.

Copy link
Member

@dshokouhi dshokouhi left a comment

Choose a reason for hiding this comment

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

Works great and hopefully clears up any remaining confusion on the prioritize internal option

@JBassett JBassett merged commit 9474675 into home-assistant:master Jul 9, 2022
@jpelgrom jpelgrom deleted the compose-home-network branch July 9, 2022 20:04
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.

None yet

4 participants