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

fix: upgrade to analytics with highcharts major version bump and address regressions #3288

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

HendrikThePendric
Copy link
Contributor

@HendrikThePendric HendrikThePendric commented Nov 12, 2024

ISSUE WAS PUT ON HOLD DUE TO KFMT FINDINGS

No issue nr available

Requires: dhis2/analytics#1728

This PR is blocked

See KFMT Finding for more details, but the following points are blocking this PR:

At the time of writing we are using a GitHub commit URL for @dhis2/analytics, once the above PR is merged we can switch to a regular NPM version number.


Key features

  1. We are now using Highcharts v11

Description

After the upgrade, we found an issue with the height of the chart: it would not fill the container but set the default 400px height. After discussing the problem with @edoardo we found that removing some code that we used to scale the chart fixed the issue. Quite likely, there was some interference between Highchart's auto-scaling and ours.

KFMT findings

During KFMT two critical issues emerged. Although a workaround exists for both of these bugs, we decided it would be better to put this PR on hold.

Highcharts v11 is incompatible with Batik

Highcharts uses feDropShadow, which is a valid SVG element but is not supported by Batik. This problem was also reported in [a Highcharts issue] (highcharts/highcharts#21557) by someone else, and a workaround solution was suggested. However, for us to implement this workaround would be quite messy since the CSS part would be in DV and the rest of the workaround in @dhis2/analytics. Besides this, there ongoing work in dhis2/analytics#1731 and #3135 to switch to Highcharts offline-export module for exports and once this is completed we do not need to rely on Batik anymore, making this problem irrelevant.

There is a bug in highcharts related to scatterplots with the boost module

We've reported the bug highcharts/highcharts#22183 and decided we would wait for it to be resolved before proceeding. We did come up with a workaround though: we could merge all data-points to a single series and style each point individually, as in this example.


TODO


Known issues

  • Highcharts 11 uses a new color scheme. It is not wildly different from the old one and looks quite nice. During KFMT we decided we would keep it. If we were to change out minds, here are some instructions on how to revert to the old theme.

@dhis2-bot
Copy link
Contributor

dhis2-bot commented Nov 12, 2024

🚀 Deployed on https://pr-3288.data-visualizer.netlify.dhis2.org

@dhis2-bot dhis2-bot temporarily deployed to netlify November 12, 2024 14:40 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify November 13, 2024 08:48 Inactive
@HendrikThePendric HendrikThePendric marked this pull request as draft November 13, 2024 09:23
@HendrikThePendric HendrikThePendric force-pushed the chore/upgrade-highcharts-in-analytics-to-v11 branch from 44f0b6e to 9f4ec91 Compare November 13, 2024 16:12
@dhis2-bot dhis2-bot temporarily deployed to netlify November 13, 2024 16:16 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify November 13, 2024 16:22 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify November 14, 2024 16:25 Inactive
@HendrikThePendric HendrikThePendric force-pushed the chore/upgrade-highcharts-in-analytics-to-v11 branch from 1224c83 to c64912f Compare November 14, 2024 16:25
@dhis2-bot dhis2-bot temporarily deployed to netlify November 14, 2024 16:27 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants