Skip to content

Weatherpanel#375

Merged
balloob merged 8 commits intohome-assistant:masterfrom
fanthos:weatherpanel
Aug 8, 2017
Merged

Weatherpanel#375
balloob merged 8 commits intohome-assistant:masterfrom
fanthos:weatherpanel

Conversation

@fanthos
Copy link
Copy Markdown
Contributor

@fanthos fanthos commented Aug 5, 2017

Replace table with Google Charts having templow and condition support.
#373

@mention-bot
Copy link
Copy Markdown

@fanthos, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Tommatheussen, @armills and @andrey-git to be potential reviewers.

@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 5, 2017

Can you include a screenshot?

@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 5, 2017

Travis also reported some lint errors that need to be fixed (the final test run will fail, that one can be ignored)

@fanthos
Copy link
Copy Markdown
Contributor Author

fanthos commented Aug 5, 2017

OpenWeatherMap:
default
YWeather:
default

@bramkragten
Copy link
Copy Markdown
Member

What is the policy for UI elements like these, last release it changed from graph to a table, now someone suggests to change it back to a (more extensive) graph, and the release after back to a table?

The weather card is the one I like least of all cards, main reason I want #365 to do this, so I don't mind a few changes, but just wondering if there is a policy.

@mjj4791
Copy link
Copy Markdown

mjj4791 commented Aug 6, 2017

Buienradar weather
I'd like to know where the weather card is going as well; I noticed the new gui element in 0.50 for the weather component, and adapted buienradar weather to that. Now I discover we are reverting that....

The (previously) new design, allowed for more diverse data to be shown (ie series of unequal units); with a graph this is more difficult....
Although I really liked the graph from before, the table provides the opportunity to add more insight....
I always like to know the forecasted temperature together with the rainchance (for example)...

What I had using the (now-old) design of the card (temperature, templow, sunchance, rainchance, rain, snow, wind and date(day)):
image
image
image
image

@fanthos
Copy link
Copy Markdown
Contributor Author

fanthos commented Aug 6, 2017

Personally I like the table more because I can see more detailed information.

@bramkragten
The reason revert the table back to chart is the table does not work well when the weather component provides per-hour forecast(like OpenWeatherMap) #373.

@mjj4791
I tested Buienradar component, it provides data similar to YWeather. Maybe make the table and chart both available and display them by which data provided by component? Or its possible show multiple lines with same X-axis on the same chart.

@bramkragten
Copy link
Copy Markdown
Member

The weather component should be more standardized anyway, like the attribute for wind bearing, the one element uses windBearing the other wind_bearing. Besides that the UI doesn't check if the attribute excists and then faulty displays North. That's just one example, for the standard attributes, that we will show in the UI, we should have standards.

@fanthos
Copy link
Copy Markdown
Contributor Author

fanthos commented Aug 6, 2017

Agree.
Right now there are no standard for forecast property names. They should be defined in Weather platform __init__.py or something, not in every component's python file. When displaying the card, it should check windBearing if exists.

@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 6, 2017

All the property names are standardized in the Weather entity abstract base class: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/weather/__init__.py#L48-L169

There has been no policy on the weather card and I should probably not have merged the previous PR because it was such a major change.

We do have guide lines for things in the frontend in general:

  • State cards should show minimal data. All extra data should be moved to a more info card (what you see when you click on an entity). Tables are definitely more info card material.
  • We don't do configurable options how things look in the frontend as it will lead to a support nightmare.

For the weather card, I wouldn't mind being inspired by the Google weather card (note that this screenshot is not their original card, I deleted some parts):

image

And actually, that is already what we get after this PR, so that's good 👍

Comment thread src/cards/ha-weather-card.html Outdated
@@ -194,6 +187,52 @@
this.windBearing = this.windBearingToText(this.stateObj.attributes.windBearing);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is no windBearing attribute. It should be wind_bearing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should also always assume that certain attributes might not be available.

@mjj4791
Copy link
Copy Markdown

mjj4791 commented Aug 6, 2017

When implementing the above mentioned change regarding wind_bearing, I would also revise this:

windBearingToText: function (degree) {
  return _DEGREE_TEXT[((parseInt(degree) + 5.63) / 11.25) | 0];
}

into something similar to this:

windBearingToText: function (degree) {
  var index = -1;
  if (!isNaN(parseInt(n)) && isFinite(n)) {
    index = parseInt((degree + 11.25)/22.5);
  }
  if (index < 0 || index >16) {
    return '';
  } else {
    return _DEGREE_TEXT[index]
  }

This changes the following:

  • do not assume N as wind direction
  • updated calculation, which selects correct direction

@fanthos
Copy link
Copy Markdown
Contributor Author

fanthos commented Aug 7, 2017

Document for more-info panel is at https://home-assistant.io/developers/frontend_add_more_info/ .

I replaced the <table> with <div>.
Currently the code makes the panel looks like
default
Should I move the forecast icon back to center?

@mjj4791
Copy link
Copy Markdown

mjj4791 commented Aug 7, 2017

Looks nice!
Just tried this version of the card; using buienradar weather component, it gives me the images shown below. (and if no wind_bearing is passed in, 'N' is no longer assumed)

and without forecast data:

@bramkragten
Copy link
Copy Markdown
Member

bramkragten commented Aug 7, 2017

Looks better in my opinion! Just make sure the icon, temperature and the 3 attributes are all aligned.

And I think mdi icons would make it look even better, and more consistent. It is easy to implement, just replace the [[nowCond]] with <iron-icon icon="mdi:[[nowCond]]"></iron-icon> and put the icon names in _WEATHER_TO_ICON...

@fanaticDavid
Copy link
Copy Markdown
Contributor

+1 for using mdi icons!

Consistency pleases my OCD 😛

@fanthos
Copy link
Copy Markdown
Contributor Author

fanthos commented Aug 8, 2017

Replace the current condition with mdi icons is not hard, but seems Google Charts does not support HTML annotation.
The forecast chart using Google chart, so the forecast chart can not use mdi icons.

@bramkragten
Copy link
Copy Markdown
Member

bramkragten commented Aug 8, 2017

@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 8, 2017

Let's leave MDI icons for a next PR and let's get this merged.

@balloob balloob merged commit 2e1d3e1 into home-assistant:master Aug 8, 2017
@fanthos fanthos deleted the weatherpanel branch September 6, 2017 17:12
tkdrob pushed a commit to tkdrob/frontend that referenced this pull request Apr 20, 2021
* Update siri-shortcuts.md

Corrections and clarifications in steps 2-4, renumbered

* Update docs/integrations/siri-shortcuts.md

* Update docs/integrations/siri-shortcuts.md

* Update docs/integrations/siri-shortcuts.md

* Update docs/integrations/siri-shortcuts.md

Co-authored-by: Tom Brien <TomBrien@users.noreply.github.com>
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 8, 2022
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.

7 participants