rusackas commented on code in PR #37107:
URL: https://github.com/apache/superset/pull/37107#discussion_r2743265453
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -123,8 +236,8 @@ class ChartRenderer extends Component {
onFilterMenuOpen: this.props.onFilterMenuOpen,
onFilterMenuClose: this.props.onFilterMenuClose,
onLegendStateChanged: this.handleLegendStateChanged,
- setDataMask: dataMask => {
- this.props.actions?.updateDataMask(this.props.chartId, dataMask);
+ setDataMask: (dataMask: DataMask) => {
+ this.props.actions?.updateDataMask?.(this.props.chartId, dataMask);
Review Comment:
This is not a regression from the TypeScript migration — it's pre-existing
behavior. The original JavaScript on `master` (`ChartRenderer.jsx:126-128`) has
the identical logic:
```js
setDataMask: dataMask => {
this.props.actions?.updateDataMask(this.props.chartId, dataMask);
},
```
The `setDataMask` prop was declared in PropTypes but was never actually used
in the hook implementation, and no caller passes it — `Chart.tsx` doesn't
include `setDataMask` in its props, so it never reaches `ChartRenderer`.
This exact concern was already raised by this bot on Jan 13, which led to an
accidental behavioral change (adding an `if/else` branch). @justinpark caught
it during review, and @rusackas correctly reverted to match the original JS
behavior. The prop declaration could be cleaned up (removed from the interface
since it's unused), but that's a separate concern from this migration PR.
--
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]
