Skip to content
This repository has been archived by the owner on Nov 4, 2023. It is now read-only.

feat(WEATHER): localized number values and support "pressure" key #666

Merged
merged 20 commits into from
Mar 13, 2021

Conversation

mikyjazz
Copy link
Contributor

  • added filters to localize numeric values according to locale
  • changes in CLIMATE tile
  • added function filterNumber in $scope to call from anonymous functions (existing function numberFilter does not use locale)

styles/main.less Outdated Show resolved Hide resolved
@rchl
Copy link
Collaborator

rchl commented Mar 11, 2021

I have a test tile that renders like this before (master) and after (this change):

Screenshot 2021-03-11 at 21 37 53

Screenshot 2021-03-11 at 21 38 04

Here is the tile's code:

                  {
                     title: 'Home',
                     position: [6, 6],
                     height: 2.5, width: 2,
                     // classes: ['-compact'], // enable this if you want a little square tile (1x1)
                     type: TYPES.WEATHER,
                     id: 'weather.home',
                     state: function () {return 'Clear, night'}, // https://github.com/resoai/TileBoard/wiki/Anonymous-functions
                     icon: 'clear-night',
                     icons: {
                        'clear-night': 'nt-clear',
                        'cloudy': 'cloudy',
                        'exceptional': 'sunny',
                        'fog': 'fog',
                        'hail': 'sleet',
                        'lightning': 'chancestorms',
                        'lightning-rainy': 'tstorms',
                        'partlycloudy': 'partlycloudy',
                        'pouring': 'rain',
                        'rainy': 'chancerain',
                        "snowy": 'snow',
                        'snowy-rainy': 'sleet',
                        'sunny': 'sunny',
                        'windy': 'hazy',
                        'windy-variant': 'flurries'
                     },
                     states: {
                        "clear-night": "Clear, night",
                        "cloudy": "Cloudy",
                        "exceptional": "Exceptional",
                        "fog": "Fog",
                        "hail": "Hail",
                        "lightning": "Lightning",
                        "lightning-rainy": "Lightning, rainy",
                        "partlycloudy": "Partly cloudy",
                        "pouring": "Pouring",
                        "rainy": "Rainy",
                        "snowy": "Snowy",
                        "snowy-rainy": "Snowy, rainy",
                        "sunny": "Sunny",
                        "windy": "Windy",
                        "windy-variant": "Windy"
                     },
                     fields: { // most of that fields are optional
                        summary: '&sensor.dark_sky_summary.state',
                        temperature: '&sensor.dark_sky_temperature.state',
                        temperatureUnit: '&sensor.dark_sky_temperature.attributes.unit_of_measurement',
                        windSpeed: '&sensor.dark_sky_wind_speed.state',
                        windSpeedUnit: '&sensor.dark_sky_wind_speed.attributes.unit_of_measurement',
                        humidity: '&sensor.dark_sky_humidity.state',
                        humidityUnit: '&sensor.dark_sky_humidity.attributes.unit_of_measurement',

                        list: [
                           // custom line
                           'Feels like '
                              + '&sensor.dark_sky_apparent_temperature.state'
                              + '&sensor.dark_sky_apparent_temperature.attributes.unit_of_measurement',

                           // another custom line
                           'Pressure '
                              + '&sensor.dark_sky_pressure.state'
                              + '&sensor.dark_sky_pressure.attributes.unit_of_measurement',

                           // yet another custom line
                           '&sensor.dark_sky_precip_probability.state'
                              + '&sensor.dark_sky_precip_probability.attributes.unit_of_measurement'
                              + ' chance of rain',
                        ],
                     },
                  },

@rchl
Copy link
Collaborator

rchl commented Mar 11, 2021

It looks like it actually looks correct with your changes. The only thing is that the windSpeed icon still has some negative margin applied.

And of course, all the sensor values are not correct but that is expected as I don't have that sensor in HA.

scripts/directives/tile.html Outdated Show resolved Hide resolved
scripts/directives/tile.html Outdated Show resolved Hide resolved
scripts/directives/tile.html Outdated Show resolved Hide resolved
fixed wrong check
@mikyjazz
Copy link
Contributor Author

for the pressure it is right to use the appropriate field with the icon I added instead of putting it in the list (also in the example)

scripts/directives/tile.html Outdated Show resolved Hide resolved
@mikyjazz
Copy link
Contributor Author

ALWAYS a space must be before units

@mikyjazz
Copy link
Contributor Author

please remove those changes

@mikyjazz
Copy link
Contributor Author

"The International System of Units (SI) prescribes inserting a space between a number and a unit of measurement (being regarded as a multiplication sign) but never between a prefix and a base unit; a space (or a multiplication dot) should also be used between units in compound units."

@rchl
Copy link
Collaborator

rchl commented Mar 12, 2021

Please find any weather website that puts space before the degrees (celsius) symbol.
I've checked the first two that come to mind and there wasn't any:

@mikyjazz
Copy link
Contributor Author

not interested in what doing others... SI is law...

@rchl
Copy link
Collaborator

rchl commented Mar 12, 2021

We're not making a government approved application here. If there is widely applied convention that is not in line with the "law" then we should go for the convention.

I'm gonna look more into it tomorrow but it doesn't seem right what you are suggesting (at least for the degrees symbol).

@rchl
Copy link
Collaborator

rchl commented Mar 12, 2021

And I'm 100% sure that you don't put space before the percentage sign. Nobody writes 100 %, or at least shouldn't.

@rchl
Copy link
Collaborator

rchl commented Mar 13, 2021

I admit that SI says that there should be a space before both the "degrees celsius" symbol and the percentage symbol. It's not a law though but a standard and in practice, that rule is not commonly followed (at least outside of scientific papers). Thus, I'm against having a space for those two.

I know you said you "not interested in what doing others" but the majority (haven't found any exceptions) of weather providers don't put space before those symbols. All of those don't have space: yr.no, openweathermap, the weather channel, google. For example:
Screenshot 2021-03-13 at 11 25 47

The original code wasn't consistent about it so I think it makes sense to make this consistent one way or another. I'm in favor of not having space. If you want to argue against it then I guess we'll have to invite a third-party (the owner of the project) to make the decision.

@alphasixtyfive
Copy link
Contributor

While ISO 31-0 dictates that there must be a non-breaking space between value and a symbol, all of the style guides that I've come across are against it. I am always driven by what actually looks and feels right and to me personally an absence of space looks more natural.

@mikyjazz
Copy link
Contributor Author

Very well, the project is yours and you can usually decide what is right or best to do. For my part, coming from the academic world, it is obviously impossible to privilege the aesthetic aspect to the detriment of the rules. Go ahead and change the spacing, I'll continue with my fork ... Just to remind you, I initially corrected the examples where "C" was reported in the temperature unit instead of "°C".
Good job!

@@ -23,7 +23,7 @@
</div>

<div class="header-weather--temperature" ng-if="item.fields.temperature">
<span ng-bind="getWeatherField('temperature', item, {})"></span>
<span ng-bind="(getWeatherField('temperature', item, {}) | number:1)"></span>
Copy link
Collaborator

@rchl rchl Mar 13, 2021

Choose a reason for hiding this comment

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

Does it actually make sense to show one decimal place for the outside temperature? I would argue that it's not because those numbers are never precise enough for that to matter. And again, I could give other weather forecasts as an example against it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In any way, this is not blocking the review, just wanted to know your opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a sensible question, all the services we normally get information from provide a decimal as precision, then it may or may not make sense to show it in the UI, I like to see it ... It would probably be nice to have the ability to configure this thing with a entry in config.json ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I'd prefer not to see it as it would make the brain "ingest" the number more easily without the decimal digits but will ignore it for now.

@rchl rchl changed the title Localized CLIMATE tile numbers and generic function for localization feat(WEATHER): localized number values and support "pressure" key Mar 13, 2021
@rchl rchl merged commit 3a1b206 into resoai:master Mar 13, 2021
@rchl
Copy link
Collaborator

rchl commented Mar 13, 2021

Thanks!

(Note that these changes didn't seem to have anything to do with the CLIMATE tile so I changed the summary accordingly).

@mikyjazz
Copy link
Contributor Author

Yes that's right, there were other changes on climate that I excluded to make the pull request only cover one topic. At this point we have a problem, tell me how we should proceed ... In my fork there will always be the space between numbers and units and the decimal when "I like" ... What do I do? Would I do pull of other changes (but then you'll need to change you those things from you) or our "partnership" ends here?

@rchl
Copy link
Collaborator

rchl commented Mar 13, 2021

I also have a "custom" branch with my local changes that don't belong upstream (here) because those are either not mature or just too specific. That shouldn't pose much of a problem for creating PRs as long as those PRs don't modify the exact code that you have customized.

You can develop the feature on your custom branch. Once you are ready to submit a PR, you can for example commit it on your custom branch, then create a new fresh branch based on upstream master and cherry-pick your change there. Should be a clean cherry-pick in most cases. Then submit that branch for review and address any potential issues on that branch.

Once the PR is merged you can switch back to your custom branch and rebase it on upstream's master. There will likely be conflicts if there were some changes made during code review but should be easy to resolve (just pick the upstream side).

(Alternatively, you could even avoid committing the initial version of your changes on your custom branch to avoid conflicts later, during rebase.)

@mikyjazz mikyjazz deleted the localized_numbers branch March 13, 2021 21:21
@alphasixtyfive
Copy link
Contributor

Very well, the project is yours and you can usually decide what is right or best to do. For my part, coming from the academic world, it is obviously impossible to privilege the aesthetic aspect to the detriment of the rules. Go ahead and change the spacing, I'll continue with my fork ... Just to remind you, I initially corrected the examples where "C" was reported in the temperature unit instead of "°C".
Good job!

I know that you are right and I very much value the time you are spending to push this project forward. I have no scientific or even programming background, I'm an accountant and most of the time whatever I do is dictated solely but a gut feeling.

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

Successfully merging this pull request may close these issues.

3 participants