Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate from chart.js to echarts #23809

Merged
merged 45 commits into from
Jan 23, 2025
Merged

Migrate from chart.js to echarts #23809

merged 45 commits into from
Jan 23, 2025

Conversation

MindFreeze
Copy link
Contributor

@MindFreeze MindFreeze commented Jan 20, 2025

Breaking change

This could break custom styling but is non breaking for HA directly

Proposed change

Remove Chart.js and replace it with Echarts

Pros:

  • Better performance with large datasets
  • Ability to render with canvas and/or SVG
  • Potential to create many more types of charts. See examples
  • Better customization
  • More feature rich
  • Code is much better written than Chart.js making contributions easier

Cons:

  • Bundle size is increased by ~1.5% . Our code is less but the lib code is more
  • Performance with very small datasets may be slightly worse but this seems anecdotal

Known current issues

  • Sometimes zoom with ctrl/cmd + scroll doesn't work until you focus(click) a chart
  • Zoom and scroll logic is different when the device supports touch but what do we do about laptops that have a touchscreen and a keyboard
  • Cursor turns to pointer when hovering over a line as mentioned by @wendevlin below. See this comment

Click to open more-info has not been migrated as it was decided to remove this from charts but we should replace it with something else.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@steverep
Copy link
Member

The proposal area of the PR form is blank. Providing some sort of write-up explaining the reasons behind the change would help, especially for such a large PR.

@MindFreeze
Copy link
Contributor Author

@steverep it's not ready. Have patience, please

@wendevlin
Copy link
Contributor

Fixing the cursor is not simple. We can set the cursor for the points but not the lines between them. See this issue. We can set cursor: default !important for the canvas but then it will affect the legend as well where we want pointer

Ah ok, yeah then I would leave it as it is.

@MindFreeze MindFreeze marked this pull request as ready for review January 22, 2025 07:04
@wendevlin
Copy link
Contributor

I still have the issues:

  • Zoom with ctrl + scroll is sometimes not taken but echarts and then my browser zooms in 🙈
    • maybe we should think about another hotkey or disable crtl+scroll for our whole page
  • On mobile it can happen that the tooltip is not readable

@MindFreeze
Copy link
Contributor Author

Should be fixed now

@MindFreeze MindFreeze added the Noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) label Jan 23, 2025
@wendevlin wendevlin enabled auto-merge (squash) January 23, 2025 15:45
@wendevlin wendevlin merged commit 1f8cfdd into dev Jan 23, 2025
11 checks passed
@wendevlin wendevlin deleted the echarts branch January 23, 2025 15:51
@karwosts
Copy link
Contributor

Would you like me to start opening issues related to this, or do you want more time to work on it and I'll wait for the beta?

Have to say this is looking pretty rough at the moment, see quite a lot of visual problems 😬

@wendevlin
Copy link
Contributor

Would you like me to start opening issues related to this, or do you want more time to work on it and I'll wait for the beta?

Have to say this is looking pretty rough at the moment, see quite a lot of visual problems 😬

Please open an issue and assign it to @MindFreeze. Thanks!

@DerDaku
Copy link

DerDaku commented Feb 11, 2025

While generally this is a nice change, it is a step back for the graphs legend.

In this discussion (#24112) I proposed a change that brings back the old legend. (DerDaku@5595194) Sadly my typescript skills are somewhat basic, so it is probably not ready to be added to home assistant (if it is even wanted). Is there anyone that could have a look at it and provide feedback?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed Noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants