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

feat: use highchart offline export for downloads #3135

Open
wants to merge 49 commits into
base: master
Choose a base branch
from

Conversation

HendrikThePendric
Copy link
Contributor

@HendrikThePendric HendrikThePendric commented Jul 9, 2024

Implements DHIS2-17722 and also fixes DHIS2-12103.

Requires dhis2/analytics#1731


Key features

  1. Use HighCharts offline export method where possible
  2. Add the correct font bundle for the user's locale, this allows exporting visualizations with non-ASCII fonts

Description

  • This solution has been implemented using the HighCharts exportChartLocal() method.
  • For PDF exports it looks at the user's DB locale to serve the correct font bundle for the user's language/script. For this to work we had to add a lot of font bundles to the public folder. This will increase bundle size, but shouldn't increase download size to the client, because the fonts are not downloaded until a PDF export takes place.
  • In order to use the HighCharts exportChartLocal() method, we need access to the current chart in the useDownload hook. Previously we just kept the SVG string in the global redux store, but now we need access to the HighCharts Chart instance which has the exportChartLocal() method. A redux store is only supposed to contain serialisable data and we were violating that principle by putting the chart instance in there, so I moved the chart into a context. (FYI: it wasn't just a matter of principle I also remember getting an error when I tried to put the Chart instance into the redux store)

Font bugs

During a KFMT session we found various font-related bugs.

  1. PNG exports are expected to look identical to the chart in the browser, but they didn't.
  2. PDF exports are expected to look slightly different since they should use the Noto Sans font. We found that they were using the Verdana font or something similar to that
  3. We found an issue with text alignment (horizontal alignment was off-centred) and text ellipsis (it was not showing)

It seems that at least in-part these three bugs are related to each other.

Bug 1: PNG font-family consistency

What I found was the following:

  • For PNG export to work correctly all nodes must inherit their font-family from the root SVG.
  • When inspecting various <text> nodes in our charts in the browser's element-inspector, we can see that the font-family is correctly set to Roboto, sans-serif, but these styles are inherited from elements such as the document body and some other selectors, not from the root SVG element.
  • When we add a style rule that forces all child elements to have the same font-family as the root SVG, the PNG export seems fixed.

(I have some elaborate theories on why this actually fixes things, but since I am not actually 100% sure, I'll refrain from going into detail)

To address this, we could add the following rule to src/components/VisualizationPlugin/styles/ChartPlugin.module.css (this file will be in the current branch after the Highcharts upgrade PR #3288 is merged:

/* This forces all SVG descendants to have the same
 * font-family as the SVG root element, which is required
 * for offline export to PNG to work correctly. */
:global(svg.highcharts-root *) {
    font-family: inherit;
}

Bug 2: PDF font-family consistency

I expect that this fix for this bug builds upon the bugfix nr 1. It's possible that it is actually solved already after implementing it. But if not, I expect the following two steps may be needed:

  1. We may need to call chart.update() and set the chart's font-family to a variety of Noto Sans
  2. If we still see issues after that, then I think an additional fix is needed, which is to make the base Noto Sans font available to the page, so that the chart can actually be rendered in this font before printing. If this is required for cases when a specific script variety of Noto Sans is used, we need to set the base font as a fallback, i.e. fontFamily: '"Noto Sans Khmer", "Noto Sans", sans-serif

Bug 3: Text alignment

We expect the text alignment issues to be resolved once the font-family issues have been tackled. Suppose that in in the fix for bug nr 2 we only need to do step 1, and we find that we still have the text alignment issue, then this could require us to take step to after all. It could be possible that Highcharts passes some measurements of the SVG to the PDF generation process, and in order for these measurements to be correct, it will need to have the correct font rendered on the page.


TODO


Known issues

  • The current solution should support all known implementations, but there are also configurations possible which would not be supported. Specifically if we encounter a language that can be written in 2 or more non-ASCII scripts that both need to be supported. The current frontend solution would actually be able to deal with this correctly, if the locale string contained a script section. However it is currently not possible to add a script to a DB locale. See DHIS2-17927
  • If we need/want to use this solution in event-charts too, then we probably need to move the script/language -> font-bundle logic to the analytics library.

Screenshots

Screenshot 2024-07-16 at 15 16 14

@dhis2-bot
Copy link
Contributor

dhis2-bot commented Jul 9, 2024

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

@dhis2-bot dhis2-bot temporarily deployed to netlify July 9, 2024 08:44 Inactive
src/components/HighchartsChartProvider.js Outdated Show resolved Hide resolved
src/components/Visualization/Visualization.js Outdated Show resolved Hide resolved
src/components/VisualizationPlugin/ChartPlugin.js Outdated Show resolved Hide resolved
@dhis2-bot dhis2-bot temporarily deployed to netlify July 9, 2024 10:18 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 9, 2024 13:25 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 11, 2024 08:56 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 15, 2024 13:59 Inactive
@HendrikThePendric HendrikThePendric force-pushed the feat/use-highcharts-clientside-downloads-DHIS2-17722 branch from 3822611 to 41e6cf4 Compare July 16, 2024 09:06
@dhis2-bot dhis2-bot temporarily deployed to netlify July 16, 2024 09:09 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 16, 2024 09:15 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 16, 2024 09:54 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 16, 2024 10:26 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 16, 2024 10:31 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 16, 2024 11:04 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 16, 2024 12:07 Inactive
@HendrikThePendric HendrikThePendric changed the title feat: add HighchartsChartProvider feat: use highchart offline export for downloads Jul 16, 2024
@HendrikThePendric

This comment was marked as resolved.

@dhis2-bot dhis2-bot temporarily deployed to netlify July 31, 2024 08:48 Inactive
@HendrikThePendric HendrikThePendric force-pushed the feat/use-highcharts-clientside-downloads-DHIS2-17722 branch from abea112 to 0b434bf Compare August 21, 2024 14:01
@dhis2-bot dhis2-bot temporarily deployed to netlify August 21, 2024 14:09 Inactive
languages: new Set([
'arq',
'ar', // Technically maps to both Arab and Syrc but we assume Arab
// 'az', (Arab + Cyrl)
Copy link
Contributor Author

@HendrikThePendric HendrikThePendric Aug 26, 2024

Choose a reason for hiding this comment

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

For languages like az that can be written in multiple non-latin scripts, it is strictly speaking impossible to establish the correct font bundle based on a language code.

In this current implementation I have commented out these entries, so for these languages the default Noto font bundle will be used for a PDF export, unless the locale had a script section specified.

We could also take an alternative approach, where we work with "default language-scripts". For example, we could say that "all/most DHIS2 implementations using the az language only use the Arab script, so we want to serve the NotoSansArabic bundle for the az language.

To switch from the first approach to the second would only mean uncommenting the line above. No other changes are required, and we would still be able to serve other fonts for this language if the locale were to include a script section (this is currently not supported but will be starting 2.42, see DHIS2-17927).

I guess you could say the current approach is implementing things purely based on the locale standards, while the second approach takes a more pragmatic approach. The downside of the second approach is that it creates a level of ambiguity since we would basically be deciding what the default script for a language should be. And this would also mean a bit more work.

I am leaning more towards the current implementation, but felt I should at least explain the alternative I considered.

Comment on lines 21 to 25
return (
findCustomNotoFont(({ scripts }) => scripts.has(script)) ??
findCustomNotoFont(({ languages }) => languages.has(language)) ??
NOTO_SANS_BASE_FONT_NAME
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the script of the locale takes precedence over the language.

@HendrikThePendric HendrikThePendric force-pushed the feat/use-highcharts-clientside-downloads-DHIS2-17722 branch 2 times, most recently from 0b434bf to 989db95 Compare August 26, 2024 12:51
@HendrikThePendric HendrikThePendric marked this pull request as ready for review August 26, 2024 12:51
@dhis2-bot dhis2-bot temporarily deployed to netlify August 26, 2024 12:54 Inactive
@HendrikThePendric HendrikThePendric force-pushed the feat/use-highcharts-clientside-downloads-DHIS2-17722 branch from 989db95 to e80039c Compare September 12, 2024 11:16
@dhis2-bot dhis2-bot temporarily deployed to netlify September 12, 2024 11:18 Inactive
@HendrikThePendric HendrikThePendric force-pushed the feat/use-highcharts-clientside-downloads-DHIS2-17722 branch from 3f9bed9 to 2e2120f Compare October 24, 2024 11:50
@dhis2-bot dhis2-bot temporarily deployed to netlify October 24, 2024 11:52 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.

4 participants