Split out global chart options from ones specific to the colour theme or chart type #3507
Conversation
| @@ -1,6 +1,6 @@ | |||
| /* this contains global options for all charts */ | |||
| class ChartOptions { | |||
| constructor(theme, title, type) { | |||
| class CommonChartOptions { | |||
There was a problem hiding this comment.
I've renamed the class and the file to reflect the fact that this is now common chart options only, and to differentiate it from the SpecificChartOptions class.
| class ChartOptions { | ||
| constructor(theme, title, type) { | ||
| class CommonChartOptions { | ||
| constructor() { |
There was a problem hiding this comment.
We no longer need to pass in the type of chart or the theme, as any options that need to check these have moved to SpecificChartOptions
| chart: { | ||
| backgroundColor: 'transparent', | ||
| style: { | ||
| fontFamily: '"OpenSans", "Helvetica Neue", arial, sans-serif', | ||
| color: '#222222', | ||
| }, | ||
| }, | ||
| legend: { |
There was a problem hiding this comment.
I've moved all the legend configuration to SpecificChartOptions, even though some of it is general, in order to avoid the merging of the options getting too complicated - it's easier to merge in all the top level values for each key in the options object.
|
Closing this MR as @precious-onyenaucheya-ons has now incorporated the changes in #3506 |
What is the context of this PR?
Demo for @precious-onyenaucheya-ons to show how we might split the chart options into ones that are global, for all chart types, and ones that need to be updated for each chart.
How to review this PR
Have a look at the code changes and test them to see if they make sense.
Either merge them to your PR if you would like to, or copy and adapt if you would prefer.
Checklist
This needs to be completed by the person raising the PR.