[GitHub] [superset] kgabryje removed a comment on pull request #19071: fix: Pivot Table Conditional Formatting Doesn't Show All Options

[GitBox <gi...@apache.org>]

kgabryje removed a comment 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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org