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

Create number formatting options on the profile panel #7925

Merged
merged 51 commits into from
Mar 28, 2021

Conversation

joshmcrty
Copy link
Contributor

Proposed change

This adds a Number Format option to the Profile panel that allows users to select how numbers are formatted in the UI. The available options are Auto (use language), comma for thousands separator, decimal for thousands separator, and none which bypasses the formatting function wherever it is used.

Screen Shot 2020-12-07 at 11 26 11 AM

This is implemented by saving the format choice to the core user data. I'm not sure if any core/python changes are required; this seems to work fine in my dev environment with frontend changes only.

This PR updates the computeStateDisplay function to use a new argument -- hass.userData -- and updates all areas of the code that call this function to pass in the new argument. I opted for this approach so that in the future if other formatting features are created (for example date format options), they can be saved to the core user data and will already be available to computeStateDisplay without updating all uses of computeStateDisplay again.

The formatNumber function is also updated with new expected arguments to allow for format options. All direct uses of the formatNumber function have been updated.

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

Go to the profile page and select a number format. Lovelace dashboards should now display numbers that reflect the selected number format.

Additional information

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:

@kukulich
Copy link
Contributor

kukulich commented Dec 7, 2020

I'm not sure if the two options (comma for thousands separator, decimal for thousands separator) make sense. Eg. in Czech we have space for thousands separator.

I'm happy user of the auto option (and I thank you very much for the implementation) but I can imagine some users may want to use English language and the Czech number formatting - just like other users for other languages in #7745

So I think the best implementation would be to have two options for decimal point and thousands separator.

@joshmcrty
Copy link
Contributor Author

@kukulich thank you for the feedback. I think you are right and I want to enable this feature to handle as many use-cases as possible. I'm going to rework the options a bit, because the current approach starts to get brittle with more and more options. Unfortunately there isn't a good browser localization API where I can arbitrarily specify what to use for the thousands separator and the decimal separator; it has to be a Unicode BCP 47 locale identifier.

@joshmcrty joshmcrty marked this pull request as draft December 8, 2020 00:08
@joshmcrty
Copy link
Contributor Author

The latest commits add an option for spaces as thousands separators, and a "Use system locale" option that uses the system default locale. This option is probably what most users should choose when they select English as the language but prefer native locale formatting.

The commits also add fallback locales for the three specific formatting options in case the first locale is not available on the users browser/operating system. This should ensure that the formatting works as expected in most cases.

A "live" formatted number is now in the description to provide quick visual feedback so users can verify the chosen format is working as expected.

Screen Shot 2020-12-07 at 6 28 30 PM

@bramkragten
Copy link
Member

bramkragten commented Jan 5, 2021

I'm thinking about where we should store these settings, currently, we already store the language per user in the frontend user data under language, we now add this to core, I think these settings should be together as they are all about localization. I think we should pass the format functions 1 object with localization options.

@spacegaier
Copy link
Member

I think I agree with Bram that this would be cleaner and just a logical:

{
    "version": 1,
    "key": "frontend.user_data_ca2aa4188cea409b8b281db15bec6072",
    "data": {
        "language": {
            "language": "en",
            "number_format": "de"
        }
    }
}

At runtime we then have this one object with all localization info, so we keep lean and clean interfaces 👍 .

@joshmcrty
Copy link
Contributor Author

I think I agree with Bram that this would be cleaner and just a logical:

{
    "version": 1,
    "key": "frontend.user_data_ca2aa4188cea409b8b281db15bec6072",
    "data": {
        "language": {
            "language": "en",
            "number_format": "de"
        }
    }
}

At runtime we then have this one object with all localization info, so we keep lean and clean interfaces 👍 .

@bramkragten @spacegaier saving the data to the language object is easy enough, but how should I access it? hass.language is just the string "en" from the language property in this example. I saved the numberFormat to the core user data because that was accessible from the hass.userData object.

@bramkragten
Copy link
Member

Yeah, it would need some refactoring

@joshmcrty joshmcrty force-pushed the profile-number-format branch 2 times, most recently from 7e95e33 to 9ec6d33 Compare January 29, 2021 02:41
@joshmcrty
Copy link
Contributor Author

@bramkragten I rebased and force pushed after all of the import sorting in dev. The latest changes include a refactor of the use of hass.language as an object instead of a string as well as addressing the comments from your previous review.

@joshmcrty
Copy link
Contributor Author

@bramkragten the latest commits should address all of the comments from your last review.

@bramkragten
Copy link
Member

Sorry for the delay, I'll look into it this week. Could you rebase and fix the tests?

@joshmcrty
Copy link
Contributor Author

@bramkragten rebase is complete and all tests are passing.

@JonahKr
Copy link

JonahKr commented Mar 3, 2021

Will this be accessible like the hass language parameter to lovelace cards?
Would be interesting to know for something like this

@joshmcrty
Copy link
Contributor Author

Will this be accessible like the hass language parameter to lovelace cards?
Would be interesting to know for something like this

@JonahKr I have not done any custom lovelace card development, so I'm not familiar with what data or methods are available to custom cards. If the card is using the hass object, then this would potentially be a breaking change because any card expecting hass.language to be a string would need to be adapted to use hass.language.language, and could also use hass.language.number_format. The custom-card-helper function you referenced should probably be updated if/when this PR is merged to match the changes made to that function here.

@bramkragten do you know how this may impact custom lovelace cards?

@bramkragten
Copy link
Member

I was thinking about that the other day, this will be a breaking change for custom cards, something we should think about.

@bramkragten
Copy link
Member

🎉 Thanks @joshmcrty

@bramkragten bramkragten merged commit f43c420 into home-assistant:dev Mar 28, 2021
@JonahKr
Copy link

JonahKr commented Mar 28, 2021

@iantrich just FYI... this will Require some changes for the format number method in Custom-Card-Helper and localize function at the Boilerplate-Card after the next release
Will create some PRs if I have time for it

@joshmcrty
Copy link
Contributor Author

@bramkragten Thanks for your patience with all of the reviews, much appreciated!

@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 2021
@joshmcrty joshmcrty deleted the profile-number-format branch April 1, 2021 13:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants