-
Notifications
You must be signed in to change notification settings - Fork 3.4k
feat: round_temperature on weather forecast card #28103
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
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| }, | ||
| "weather": { | ||
| "attributes": { | ||
| "dew_point": "Dew point", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also noticed that this was missing before in the translation file, leading to an error in my change at https://github.com/home-assistant/frontend/pull/28103/files#diff-4f7195942cc7cdfbd5ad6149411b2d6fb641035063b8711026e641d7f1a2a606R276
|
If the option is a number instead of a boolean, it would make it more flexible but I wonder if setting to 2+ digits (15.23 C) would be useful at all 🤔 |
|
@MindFreeze yeah, at first I had implemented that option but came to the same conclusion as you. it was specially confusing inside the editor, because there was a slider / number input for this option. nonetheless, I kept this logic inside the card, in case this should be changed in the future. Whatever the |
|
@MindFreeze thank you for the review, I've tried to correct all issues |
| if (!tempHigh || fc.temperature > tempHigh) { | ||
| tempHigh = fc.temperature; | ||
| tempHigh = round(fc.temperature, temperatureFractionDigits); | ||
| } | ||
| if (!tempLow || (fc.templow && fc.templow < tempLow)) { | ||
| tempLow = fc.templow; | ||
| if (fc.templow !== undefined && (!tempLow || fc.templow < tempLow)) { | ||
| tempLow = round(fc.templow, temperatureFractionDigits); | ||
| } | ||
| if (!fc.templow && (!tempLow || fc.temperature < tempLow)) { | ||
| tempLow = fc.temperature; | ||
| tempLow = round(fc.temperature, temperatureFractionDigits); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When round_temperature is false (the default), temperatureFractionDigits is undefined. The round() function has a default precision of 2. if temperatureFractionDigits is undefined we shouldn't use round to preserve the original precision
| const TEMPERATURE_ATTRIBUTES = [ | ||
| "temperature", | ||
| "apparent_temperature", | ||
| "dew_point", | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better put this constant outside of the class so it's global
Proposed change
I noticed there's no ease way to round the temperature in the weather forecast card to the nearest integers, so I added the
round_temperaturefunctionality to it. This is a boolean and when enabled is used to round all temperature numbers to their nearest integer.Type of change
Example configuration
Additional information
I used some checks to make sure that only temperatures are being rounded. From my testing this works for all values, including secondary_info's. For example, pressure is not being rounded, even with this turned on.
Before (or with `round_temperature: false)
After (only with `round_temperature: true)
Checklist
If user exposed functionality or configuration variables are added/changed: