kgabryje commented on pull request #19071:
URL: https://github.com/apache/superset/pull/19071#issuecomment-1063078461
@diegomedina248 Thanks for contribution! The problem with
`shouldMapStateToProps` always returning `True` is that we'll run
`mapStateToProps` on every change. We actually only need to update the
conditional formatting component when metrics change. We can achieve that by
using `rerender` field, like so:
```
---
a/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx
+++
b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx
@@ -66,6 +66,7 @@ const config: ControlPanelConfig = {
config: {
...sharedControls.metrics,
validators: [validateNonEmpty],
+ rerender: ['conditional_formatting'],
},
},
],
```
That will trigger rerendering of `conditional_formatting` each time
`metrics` value changes. You can check out how that works in Table chart - at
least 1 of the fields `metrics`, `groupby`, `percentage_metrics` must have a
value and they all rerender each other to determine if error message should be
displayed.
I do agree that determining if `mapStateToProps` should be run based on
number of arguments is very brittle and needs to be improved. In my opinion
your approach handles it well, I think we should keep it! But in the case of
pivot table it's not necessary and rerendering when metrics change seems to be
sufficient.
--
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]
