Yevgenium / weather-chart-card

Custom weather card with charts
MIT License
275 stars 81 forks source link

Bump to ChartJs3 (mandatory for 2021.7). Fixes #59 #60

Closed koying closed 3 years ago

koying commented 3 years ago

ChartJs was bumped to 3.X in HA 2021.7, leading to the need of numerous changes for the chart to display. Not retro-compatible at all, so it might be desirable to create a branch with the < 2021.7 version beforehand.

Mariusthvdb commented 3 years ago

great work, looking good! thank you very much!

allow me a few tiny remarks: you've added the const chartType = 'line'; and still add that(the type: ) to the chartData in the data sets. because we have a bar graph, and set the type: there, my personal preferences would be to set the type: for each chart in the chartData.

hence we can take out the const chartType, which is what I have done locally. Works perfectly. because of that, we can should also take it out off the <ha-chart-base data="[[ChartData]]" options="[[ChartOptions]]" ></ha-chart-base> line and this.ChartType = chartType; can be taken out.

Maybe personal preference, so no big deal.

lastly, Ive checked and the references to the xAxisID in the chartData do nothing? took them out, and all remains the same....

Mariusthvdb commented 3 years ago

you might want to also ditch the hours/minutes in the tooltips?

Mariusthvdb commented 3 years ago

have you seen my post in the community, especially on the language setting? Locale didnt work, (and didnt for a long time, dont understand how we could have missed that...)

I've adapted to

  set hass(hass) {
    this._hass = hass;
    this.lang = this.config.locale || this._hass.language;
    this.weatherObj = this.config.weather in hass.states ? hass.states[this.config.weather] : null;
    this.sunObj = 'sun.sun' in hass.states ? hass.states['sun.sun'] : null;
    this.tempObj = this.config.temp in hass.states ? hass.states[this.config.temp] : null;
    this.forecast = this.weatherObj.attributes.forecast.slice(0,9);
    this.windBearing = this.weatherObj.attributes.wind_bearing;
  }

or

    this.lang = this.config.locale in hass.states ? hass.states[this.config.locale] : this._hass.language;

to make it work again nicely. Not sure of we should also do that in the drawChart() because all settings are already localized now. maybe that can even be taken out of there? I guess we can, because Ive seen no issues doing so ;-)

lastly:

                title: function(context) {
                  var label = context.dataset.label || '';
                  return label += ': ' + context.parsed.y + tempUnit;
                },

can be taken out of the tooltips

koying commented 3 years ago

There is no locale in the config, is it? Isn't that a customization of yours?

Yevgenium commented 3 years ago

Please let me know when it is ready to be merged.

Mariusthvdb commented 3 years ago

There is no locale in the config, is it?

yes there is, how else would these locales be used in the card. I can't remember when this was added, but I have had

      type: custom:weather-card-chart
      title: Woensdrecht
      weather: weather.buienradar
      temp: sensor.buienradar_feel_temperature
      locale: nl

since forever?

Isn't that a customization of yours?

No I dont think it is

Mariusthvdb commented 3 years ago

Please let me know when it is ready to be merged.

glad you're back!

koying commented 3 years ago

how else would these locales be used in the card

The card currently uses the user language, and there is no "locale" here https://github.com/Yevgenium/lovelace-weather-card-chart/blob/master/weather-card-chart.js#L234 Is you version available for comparison?

Mariusthvdb commented 3 years ago

As said, it never worked like that in my setup, because it nowhere imports any of those languages?

Hence the addition of my locale setting.

Ive posted all here https://community.home-assistant.io/t/lovelace-weather-card-with-chart/88816/185?u=mariusthvdb including a link to my gist

koying commented 3 years ago

Maybe it's not needed anymore? I switched my user to NL and it seems fine image

Mariusthvdb commented 3 years ago

I have to check that, don't understand what's happening ....

Bye, since you're at it, check the top left icon position. I made a Pr for that ages ago, so maybe a good time now to take it in 1 go

Mariusthvdb commented 3 years ago

Maybe it's not needed anymore?

I switched my user to NL and it seems fine

image

Still don't really understand how it worked before, but you made me remind why this was used: to be able to override user language. Eg: I use English HA settings, but want this card to be in Dutch.

It checks if a locale is set in config, if not it selects the user language. Defaulting to English.

Yevgenium commented 3 years ago

Teaser for the new refactored version. Work in the progress... image

image

scstraus commented 3 years ago

Teaser for the new refactored version. Work in the progress... image

image

Nice. But I miss the bar graphs for rain. I can't really read the tiny numbers on there.

Mariusthvdb commented 3 years ago

yes, I tend to agree with that, looks nice at first glance but has some discussion points... maybe we should keep this issue to its topic, and first merge @koying 's PR, now that it's up to speed and has fixed all we needed to fix?

feel the new design should be in its own branch.

Yevgenium commented 3 years ago

Thanks!