fix: Pivot Table Conditional Formatting Doesn't Show All Options #19071
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19071 +/- ##
=======================================
Coverage 66.50% 66.50%
=======================================
Files 1642 1643 +1
Lines 63442 63466 +24
Branches 6439 6450 +11
=======================================
+ Hits 42191 42207 +16
- Misses 19582 19589 +7
- Partials 1669 1670 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@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.
|
@kgabryje thanks for that clarification & explanation. |
| @@ -108,6 +108,9 @@ const all_columns: typeof sharedControls.groupby = { | |||
| optionRenderer: c => <ColumnOption showType column={c} />, | |||
| valueRenderer: c => <ColumnOption column={c} />, | |||
| valueKey: 'column_name', | |||
| shouldMapStateToProps() { | |||
There was a problem hiding this comment.
Do we need it here and in dnd_all_columns and percent_metrics? mapStateToProps has only 2 arguments, so we do not need to run it every time
There was a problem hiding this comment.
No, you're correct, treated the object as two params
) * fix: Pivot Table Conditional Formatting Doesn't Show All Options * PR comments * PR comments (cherry picked from commit 0e0bece)
mistercrunch
added
🍒 1.5.0
🍒 1.5.1
🍒 1.5.2
🍒 1.5.3
🏷️ bot
SUMMARY
The Pivot Table conditional formatting was not updating the metric list when changes where made.
The problem is that the conditional formatting only executed the
mapStateToPropsfunction once, on startup, and thus, changing the information on the metrics without triggering a rerender in any other way caused the panel to have obsolete data.This pointed to a brittle way the panel were recomputing their properties: It was only done if the number of arguments of the
mapStateToPropsequals to three.Instead, this PR adds a function
shouldMapStateToPropsto the panel config interface to allow controls to request updates if they need to.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2022-03-08.at.17.24.59.mov
TESTING INSTRUCTIONS
Ensure the second added metric displays in the conditional formatting panel.
ADDITIONAL INFORMATION