-
Notifications
You must be signed in to change notification settings - Fork 0
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
Charts spike WIP #95
Charts spike WIP #95
Conversation
41e4bcc
to
b0c4d72
Compare
- Create a macro that could be repurposed by the design system - Tidy up and refactor all the JavaScript - Don't include the annotations in the chart config from the back-end, and instead build this in the JavaScript to create the arrows - Fix missing annotations JS
b0c4d72
to
fc4fdcc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, Helen. My questions are mostly for my own education, no major recommendations.
I agree that your changing of the Python data structure for annotations to something less Highcharts-specific is an improvement.
annotationConfig.labels.push({ | ||
text: annotation.text, | ||
point: { | ||
x: annotation.xValue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: I was uncertain how this would work when a bar or column chart uses categories for the x-axis, instead of a scale. It looks like the ticks are shifted, but the plots stay in the same place, and the x-index is used to generate the horizontal coordinate, so all is fine. Does this continue to work when we have categories, text labels, in the data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs further work for bar charts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I've tested with categories, and I think it works - but annotations need more work generally.
@@ -0,0 +1,37 @@ | |||
{% macro onsChart(params) %} | |||
<div data-highcharts-base-chart data-highcharts-type="{{ params.chartType }}" data-highcharts-theme="{{ params.theme }}" data-highcharts-title="{{ params.title }}" {% if params.use_stacked_layout %}data-highcharts-use-stacked-layout{% endif %}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: What's the motivation behind this change? Is it to have one macro for any charts at all? Or just one macro for highcharts for now? I'm more asking about the naming, I guess, because it's named agnostically, but contains highcharts related data attributes throughout.
Another possibility is that the role of the highcharts template is to convert our bespoke internal data structure (annotations here, data there, metadata and titles from the page…) into a Highcharts-specific format, but that too is being done in the macro file.
What am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the design system component is built, everything in the _macro.njk
will live in the design system repo, and our html template will call that. The naming is just how the design system names all their macro files, with context coming from the directory structure, e.g. https://github.com/ONSdigital/design-system/tree/main/src/components/accordion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I could have explained that better by identifying the subject. I was asking about the onsChart
macro name, not the _macro.njk
file name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see - I think it will be down to the DS team to decide on the final name for that - but you're right it might be onsHighchart or similar.
What is the context of this PR?
How to review
Check out in your local build.
Follow-up Actions