Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

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

Merged
merged 6 commits into from
Jul 13, 2021

Conversation

koying
Copy link
Contributor

@koying koying commented Jul 12, 2021

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.

@koying koying changed the title Bump to ChartJs3 (mandatory for 2021.7) Bump to ChartJs3 (mandatory for 2021.7). Fixes #59 Jul 12, 2021
@Mariusthvdb
Copy link

Mariusthvdb commented Jul 12, 2021

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
Copy link

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

@Mariusthvdb
Copy link

Mariusthvdb commented Jul 12, 2021

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
Copy link
Contributor Author

koying commented Jul 12, 2021

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

@Yevgenium
Copy link
Owner

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

@Mariusthvdb
Copy link

Mariusthvdb commented Jul 12, 2021

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
Copy link

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

glad you're back!

@koying
Copy link
Contributor Author

koying commented Jul 12, 2021

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
Copy link

Mariusthvdb commented Jul 12, 2021

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
Copy link
Contributor Author

koying commented Jul 12, 2021

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

@Mariusthvdb
Copy link

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
Copy link

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
Copy link
Owner

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

image

@scstraus
Copy link
Contributor

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
Copy link

Mariusthvdb commented Jul 13, 2021

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 Yevgenium merged commit dc2b21a into Yevgenium:master Jul 13, 2021
@Yevgenium
Copy link
Owner

Thanks!

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.

4 participants