qf-jonathan commented on code in PR #37488:
URL: https://github.com/apache/superset/pull/37488#discussion_r2754643129
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -153,26 +153,10 @@ class ChartRenderer extends Component {
this.mutableQueriesResponse = cloneDeep(nextProps.queriesResponse);
}
- // Check if any matrixify-related properties have changed
- const hasMatrixifyChanges = () => {
- const isMatrixifyEnabled =
- nextProps.formData.matrixify_enable_vertical_layout === true ||
- nextProps.formData.matrixify_enable_horizontal_layout === true;
- if (!isMatrixifyEnabled) return false;
-
- // Check all matrixify-related properties
- const matrixifyKeys = Object.keys(nextProps.formData).filter(key =>
- key.startsWith('matrixify_'),
- );
-
- return matrixifyKeys.some(
- key => !isEqual(nextProps.formData[key], this.props.formData[key]),
- );
- };
Review Comment:
@msyavuz I discovered a bug where undoing changes to controls with
`renderTrigger: true` (like `normalized`, `linear_color_scheme`, `show_values`,
etc.) wasn't causing the chart to re-render, particularly when undoing to the
initial state. The root cause was that `shouldComponentUpdate` only checked a
hardcoded list of 3 formData properties (`color_scheme`, `stack`,
`subcategories`), missing dozens of other renderTrigger controls.
The fix replaces all individual property checks with a single deep
comparison: `!isEqual(nextProps.formData, this.props.formData)`. I think this
is better because: from my understanding formData is the source of truth for
chart rendering right? if it changes, the chart should re-render, also it's a
general solution that automatically works for all current and future controls,
and also I think it's simpler and more maintainable than tracking individual
properties. The `hasMatrixifyChanges()` function becomes redundant since
`isEqual()` already detects all matrixify field changes as part of the complete
formData comparison.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
