-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix decimalSymbol for forecast #2534
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2534 +/- ##
========================================
Coverage 39.90% 39.90%
========================================
Files 21 21
Lines 1629 1629
========================================
Hits 650 650
Misses 979 979 Continue to review full report at Codecov.
|
modules/default/weather/forecast.njk
Outdated
@@ -14,10 +14,10 @@ | |||
{% endif %} | |||
<td class="bright weather-icon"><span class="wi weathericon wi-{{ f.weatherType }}"></span></td> | |||
<td class="align-right bright max-temp"> | |||
{{ f.maxTemperature | roundValue | unit("temperature") }} | |||
{{ f.maxTemperature | roundValue | decimalSymbol | unit("temperature") }} |
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.
Not sure if the order is important here... In the current njk the decimalSymbol is applied last...
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 tried that first, but it didn't work.
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 checked it again. In the forcast part it doesn't work if decimalSymbol
is at the end.
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.
How doesnt it work on your side? It looks ok if I move the dcimalSymbol to the end.
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.
Apparently I made the same mistake twice (insert the decimalSymbol inside the unit brackets). That couldn't work 🤦♂️. So it (decimalSymbol at the end) works if you do it right. Sorry.
Should I adjust my PR so that it is consistent with the current part?
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.
Yes, I think consistency is king :-) Have a good monday!
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 changed it 🙂
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.
Looks good, thx for the quick PR
With this, the
decimalSymbol
config also works at the forecast part of the new weather module.This solves the issue #2530.