Dynamic configuration and visual props #29
Conversation
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against e7a234d |
| ], | ||
| ]; | ||
| if (visualProps?.length !== 0) { | ||
| if (visualProps?.accordion?.datalabels) { |
There was a problem hiding this comment.
whats the value in this change? I see no difference?
| @@ -367,7 +392,30 @@ const renderChart = async (ctx: CustomChartContext): Promise<void> => { | |||
| }, | |||
| ], | |||
| }, | |||
| ], | |||
| ]; | |||
| if (visualProps?.length !== 0) { | |||
There was a problem hiding this comment.
this is not an array but an object right?
There was a problem hiding this comment.
element is an array, visualPropEditordefinition is an Object
| @@ -174,11 +182,8 @@ function render(ctx: CustomChartContext) { | |||
| plugins: { | |||
| // Change options for ALL labels of THIS CHART | |||
| datalabels: { | |||
| display: allowLabels ? 'auto' : false, | |||
There was a problem hiding this comment.
we seem to have removed a lot of properties... is this working as expected?
| ]; | ||
| if (yColumns?.columns.length) { | ||
| for (let i = 0; i < yColumns.columns.length; i++) { | ||
| configDefinition[0].columnSections.push({ |
There was a problem hiding this comment.
where are we using this?
There was a problem hiding this comment.
This I have added for an example purpose, when we are adding measures to y-axis, a new layer will be created with the same.
| @@ -21,10 +21,14 @@ import { | |||
| AppConfig, | |||
| ChartConfig, | |||
| ChartModel, | |||
| SuccessValidationResponse, | |||
There was a problem hiding this comment.
can you help resolve dependency cycles?
There was a problem hiding this comment.
To resolve this we need to break the types structure which we have defined
| @@ -37,7 +40,7 @@ const visualPropKeyMap = { | |||
| 2: 'accordion.datalabels', | |||
| }; | |||
|
|
|||
| const numberFormatter = value => { | |||
| const numberFormatter = (value) => { | |||
There was a problem hiding this comment.
pls add type for value
| const labelColor = _.get( | ||
| chartModel.visualProps, | ||
| visualPropKeyMap[1], | ||
| 'blue', |
There was a problem hiding this comment.
if blue is constant, can we pick from a constant?
| color: 'blue', | ||
| textStrokeColor: 'white', | ||
| textStrokeWidth: 5, | ||
| display: allowLabels, |
There was a problem hiding this comment.
display takes true now?
There was a problem hiding this comment.
https://chartjs-plugin-datalabels.netlify.app/guide/positioning.html#rotation
will depend on implementation, true is also a valid value as mentioned above in the documentation
| formatter: value => numberFormatter(value), | ||
| color: 'blue', | ||
| textStrokeColor: 'white', | ||
| textStrokeWidth: 5, |
There was a problem hiding this comment.
how are these properties working today?
| key: 'y', | ||
| label: 'Custom Y Axis', | ||
| renderChart: (ctx) => renderChart(ctx), | ||
| chartConfigEditorDefinition: ( |
There was a problem hiding this comment.
for backward compatibility verification should we keep this as it is?
There was a problem hiding this comment.
I have checked the backwards compatibilty it is working for both object and function we can keep it like this for a sample chart.
| @@ -8,6 +8,7 @@ | |||
| * Copyright: ThoughtSpot Inc. 2023 | |||
| */ | |||
|
|
|||
| import { VisualPropEditorDefinition } from '@thoughtspot/ts-chart-sdk'; | |||
There was a problem hiding this comment.
can we create a copy of this example with ur new changes? In that way, we can make sure the backward compatibility validation is in place for each version?
src/main/custom-chart-context.ts
Outdated
| if (this.chartContextProps.validateConfig) { | ||
| const validationResponse = | ||
| this.chartContextProps.validateConfig( | ||
| payload.chartConfig, | ||
| this.chartModel, | ||
| ); | ||
| if (validationResponse.isValid) { | ||
| const currentConfigState = { | ||
| config: { | ||
| ...this.chartModel.config, | ||
| chartConfig: payload.chartConfig, | ||
| }, | ||
| }; | ||
| const chartConfigEditorDefinition = | ||
| this.getChartConfigEditorDefinition( | ||
| currentConfigState, | ||
| ); | ||
|
|
||
| const visualPropEditorDefinition = | ||
| this.getVisualPropEditorDefinition( | ||
| currentConfigState, | ||
| ); | ||
| return { | ||
| ...validationResponse, | ||
| visualPropEditorDefinition, | ||
| chartConfigEditorDefinition, | ||
| }; | ||
| } |
There was a problem hiding this comment.
duplicate code for TSToChartEvent.VisualPropsValidate event.
Should we move and reuse?
No description provided.