resoai / TileBoard

A simple yet highly configurable Dashboard for HomeAssistant
MIT License
1.63k stars 278 forks source link

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

Closed mikyjazz closed 3 years ago

mikyjazz commented 3 years ago
rchl commented 3 years ago

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 commented 3 years ago

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.

mikyjazz commented 3 years ago

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)

mikyjazz commented 3 years ago

ALWAYS a space must be before units

mikyjazz commented 3 years ago

please remove those changes

mikyjazz commented 3 years ago

"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 commented 3 years ago

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 commented 3 years ago

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

rchl commented 3 years ago

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 commented 3 years ago

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

rchl commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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!

rchl commented 3 years ago

Thanks!

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

mikyjazz commented 3 years ago

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 commented 3 years ago

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.)

alphasixtyfive commented 3 years ago

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.