Set up highchart #3506
Set up highchart #3506
Conversation
✅ Deploy Preview for ons-design-system-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
src/components/charts/_macro.njk
Outdated
| </figure> | ||
|
|
||
| <!-- Set scripts to pass the config values as json to the javascript --> | ||
| {# <script type="application/json" data-highcharts-config--{{ params.uuid }}> |
There was a problem hiding this comment.
Hey @precious-onyenaucheya-ons, quick question before doing a more full review: is this meant to be commented out? It's causing a console error for me when I view the component in a local build, and the chart won't render as it can't read the content.
There was a problem hiding this comment.
Hi @precious-onyenaucheya-ons thank you for this - let's have a look through in our call this afternoon. I am also going to do some work to refine the line chart ticket tomorrow so we can get a better idea of what work is still to be done.
| align: 'left', | ||
| verticalAlign: 'top', | ||
| layout: 'horizontal', | ||
| symbolWidth: type === 'line' ? 20 : 12, |
There was a problem hiding this comment.
This is an area where I think re-working of my original code is needed. Ideally we'd set the Highcharts global options just once per page - perhaps checking with a window variable whether or not it has been done. This means that any configuration that is specific to a chart type needs splitting out and setting differently. Would it be helpful for me to help with that?
There was a problem hiding this comment.
(this could potentially be in a separate ticket)
There was a problem hiding this comment.
See my PR for how this could be done - #3507
src/components/charts/_macro.njk
Outdated
| @@ -9,6 +9,7 @@ | |||
| <figure id="{{ params.figId }}"> | |||
| <h3>{{ params.title }}</h3> | |||
| <h4>{{ params.subtitle }}</h4> | |||
| <h5 class="ons-u-vh">{{ params.description }}</h5> | |||
| @@ -37,7 +37,7 @@ class ChartOptions { | |||
| fontWeight: 'normal', | |||
| }, | |||
| }, | |||
| // chart title is rendered in wagtail | |||
| // Remove chart title | |||
There was a problem hiding this comment.
Suggestion: // Remove the chart title as rendered by Highcharts, as this is rendered in the surrounding component
What is the context of this PR?
Describe what you have changed and why, link to other PRs or Issues as appropriate.
How to review this PR
Describe the steps required to test the changes (include screenshots if appropriate).
Checklist
This needs to be completed by the person raising the PR.