Dynamic config + visual props POC #26
Conversation
src/main/custom-chart-context.ts
Outdated
| this.chartModel, | ||
| payload.visualProps, | ||
| payload.chartConfigEditorDefinition, | ||
| ); |
There was a problem hiding this comment.
this can also change the configEditorDefinition
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 2f0bf60 |
| import { | ||
| ChartConfigEditorDefinition, | ||
| ConfigEditorDefinitionSetter, | ||
| } from '../types/configurator.types'; |
There was a problem hiding this comment.
I think there is dependency cycle issue due to the imports that we have.
Please resolve this.
Also would be good to add dependency cycle lint in the current codebase.
src/main/custom-chart-context.ts
Outdated
| * @param {VisualProps} | ||
| * @returns {ChartConfigEditorDefinition[]} | ||
| */ | ||
| public getChartConfigEditorDefinition = ({ |
There was a problem hiding this comment.
This cannot be public function.
src/main/custom-chart-context.ts
Outdated
| * @param {VisualProps} | ||
| * @returns {VisualPropEditorDefinition} | ||
| */ | ||
| public getVisualPropEditorDefinition = ({ |
src/main/custom-chart-context.ts
Outdated
| if (validationResponse.isValid) { | ||
| const updatedState = { | ||
| updatedChartConfig: | ||
| this.chartModel.config?.chartConfig, |
There was a problem hiding this comment.
This should be empty as this has not changed.
There was a problem hiding this comment.
But for getting latest definition we need to pass both visual props as well as chart config
There was a problem hiding this comment.
Its good to pass both chartConfig as well as visual props for fetching the definition because either it is visual update or config update, we are trigerring the setter for both config definition and visual definition. It would be unnecessary confusion for user while defining the definition function that which one to prefer(visual props from arg or from ctx).
Will change the naming convention here for better understanding
src/main/custom-chart-context.ts
Outdated
| if (validationResponse.isValid) { | ||
| const updatedState = { | ||
| updatedChartConfig: payload.chartConfig, | ||
| updatedVisualProps: this.chartModel?.visualProps, |
There was a problem hiding this comment.
for getting latest definition we need to pass both visual props as well as chart config.
User can extract visualProps here from ctx but if we pass it explicitly it would be more convenient for dev.
src/main/custom-chart-context.ts
Outdated
| @@ -776,13 +864,21 @@ export class CustomChartContext { | |||
| this.chartModel, | |||
| ); | |||
| } | |||
| const defaultState = { | |||
There was a problem hiding this comment.
we should only send this when these are updated.
No description provided.