You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/03/08 20:36:47 UTC

[GitHub] [superset] diegomedina248 opened a new pull request #19071: fix: Pivot Table Conditional Formatting Doesn't Show All Options

diegomedina248 opened a new pull request #19071:
URL: https://github.com/apache/superset/pull/19071


   ### 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 `mapStateToProps` function 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 `mapStateToProps` equals to three.
   Instead, this PR adds a function `shouldMapStateToProps` to the panel config interface to allow controls to request updates if they need to.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   https://user-images.githubusercontent.com/17252075/157319984-ea4168bd-ee11-41eb-9f28-ecf98f97da84.mov
   
   ### TESTING INSTRUCTIONS
   1. Create a pivot table
   2. Add one metric
   3. In Customize, select that metric for conditional formatting
   4. Add a second metric
   5. Try to select the metric in conditional formatting
   
   Ensure the second added metric displays in the conditional formatting panel.
   
   ### ADDITIONAL INFORMATION
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [X] Introduces new feature or API
   - [X] Removes existing feature or API
   


-- 
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

[GitHub] [superset] diegomedina248 commented on a change in pull request #19071: fix: Pivot Table Conditional Formatting Doesn't Show All Options

Posted by GitBox <gi...@apache.org>.
diegomedina248 commented on a change in pull request #19071:
URL: https://github.com/apache/superset/pull/19071#discussion_r822844017



##########
File path: superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx
##########
@@ -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() {

Review comment:
       No, you're correct, treated the object as two params




-- 
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

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

Posted by 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

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

Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #19071:
URL: https://github.com/apache/superset/pull/19071#discussion_r822841418



##########
File path: superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx
##########
@@ -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() {

Review 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




-- 
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

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

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19071:
URL: https://github.com/apache/superset/pull/19071#issuecomment-1062273920


   # [Codecov](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#19071](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8d23da3) into [master](https://codecov.io/gh/apache/superset/commit/f9c7405e0eacf69b79063c441aa66d74c8fa3f91?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f9c7405) will **decrease** coverage by `0.00%`.
   > The diff coverage is `30.76%`.
   
   > :exclamation: Current head 8d23da3 differs from pull request most recent head 6ed0c68. Consider uploading reports for the commit 6ed0c68 to get more accurate results
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/19071/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #19071      +/-   ##
   ==========================================
   - Coverage   66.50%   66.49%   -0.01%     
   ==========================================
     Files        1642     1642              
     Lines       63442    63454      +12     
     Branches     6439     6440       +1     
   ==========================================
   + Hits        42191    42195       +4     
   - Misses      19582    19590       +8     
     Partials     1669     1669              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `51.22% <30.76%> (-0.01%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...d/packages/superset-ui-chart-controls/src/types.ts](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3R5cGVzLnRz) | `100.00% <ø> (ø)` | |
   | [...ns/plugin-chart-echarts/src/Radar/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvUmFkYXIvY29udHJvbFBhbmVsLnRzeA==) | `30.00% <0.00%> (-3.34%)` | :arrow_down: |
   | [...ugin-chart-pivot-table/src/plugin/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtcGl2b3QtdGFibGUvc3JjL3BsdWdpbi9jb250cm9sUGFuZWwudHN4) | `11.11% <0.00%> (-1.39%)` | :arrow_down: |
   | [...nd/plugins/plugin-chart-table/src/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtdGFibGUvc3JjL2NvbnRyb2xQYW5lbC50c3g=) | `15.49% <0.00%> (-1.18%)` | :arrow_down: |
   | [.../src/explore/components/ControlPanelsContainer.tsx](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sUGFuZWxzQ29udGFpbmVyLnRzeA==) | `77.77% <66.66%> (+0.11%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f9c7405...6ed0c68](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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

[GitHub] [superset] codecov[bot] commented on pull request #19071: fix: Pivot Table Conditional Formatting Doesn't Show All Options

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #19071:
URL: https://github.com/apache/superset/pull/19071#issuecomment-1062273920


   # [Codecov](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#19071](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8d23da3) into [master](https://codecov.io/gh/apache/superset/commit/f9c7405e0eacf69b79063c441aa66d74c8fa3f91?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f9c7405) will **decrease** coverage by `0.00%`.
   > The diff coverage is `30.76%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/19071/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #19071      +/-   ##
   ==========================================
   - Coverage   66.50%   66.49%   -0.01%     
   ==========================================
     Files        1642     1642              
     Lines       63442    63454      +12     
     Branches     6439     6440       +1     
   ==========================================
   + Hits        42191    42195       +4     
   - Misses      19582    19590       +8     
     Partials     1669     1669              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `51.22% <30.76%> (-0.01%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...d/packages/superset-ui-chart-controls/src/types.ts](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3R5cGVzLnRz) | `100.00% <ø> (ø)` | |
   | [...ns/plugin-chart-echarts/src/Radar/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvUmFkYXIvY29udHJvbFBhbmVsLnRzeA==) | `30.00% <0.00%> (-3.34%)` | :arrow_down: |
   | [...ugin-chart-pivot-table/src/plugin/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtcGl2b3QtdGFibGUvc3JjL3BsdWdpbi9jb250cm9sUGFuZWwudHN4) | `11.11% <0.00%> (-1.39%)` | :arrow_down: |
   | [...nd/plugins/plugin-chart-table/src/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtdGFibGUvc3JjL2NvbnRyb2xQYW5lbC50c3g=) | `15.49% <0.00%> (-1.18%)` | :arrow_down: |
   | [.../src/explore/components/ControlPanelsContainer.tsx](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sUGFuZWxzQ29udGFpbmVyLnRzeA==) | `77.77% <66.66%> (+0.11%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f9c7405...8d23da3](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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

[GitHub] [superset] rusackas merged pull request #19071: fix: Pivot Table Conditional Formatting Doesn't Show All Options

Posted by GitBox <gi...@apache.org>.
rusackas merged pull request #19071:
URL: https://github.com/apache/superset/pull/19071


   


-- 
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

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

Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #19071:
URL: https://github.com/apache/superset/pull/19071#discussion_r822841418



##########
File path: superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx
##########
@@ -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() {

Review comment:
       Do we need it here and in other controls in Table? `mapStateToProps` has only 2 arguments, so we do not need to run it every time




-- 
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

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

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19071:
URL: https://github.com/apache/superset/pull/19071#issuecomment-1062273920


   # [Codecov](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#19071](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6ed0c68) into [master](https://codecov.io/gh/apache/superset/commit/f9c7405e0eacf69b79063c441aa66d74c8fa3f91?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f9c7405) will **increase** coverage by `0.00%`.
   > The diff coverage is `44.44%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/19071/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@           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     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `51.24% <44.44%> (+0.01%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...d/packages/superset-ui-chart-controls/src/types.ts](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3R5cGVzLnRz) | `100.00% <ø> (ø)` | |
   | [...ns/plugin-chart-echarts/src/Radar/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvUmFkYXIvY29udHJvbFBhbmVsLnRzeA==) | `30.00% <0.00%> (-3.34%)` | :arrow_down: |
   | [...ugin-chart-pivot-table/src/plugin/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtcGl2b3QtdGFibGUvc3JjL3BsdWdpbi9jb250cm9sUGFuZWwudHN4) | `12.50% <ø> (ø)` | |
   | [...nd/plugins/plugin-chart-table/src/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtdGFibGUvc3JjL2NvbnRyb2xQYW5lbC50c3g=) | `16.17% <0.00%> (-0.50%)` | :arrow_down: |
   | [.../src/explore/components/ControlPanelsContainer.tsx](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sUGFuZWxzQ29udGFpbmVyLnRzeA==) | `77.77% <66.66%> (+0.11%)` | :arrow_up: |
   | [...frontend/src/SqlLab/components/SqlEditor/index.jsx](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci9pbmRleC5qc3g=) | `50.84% <0.00%> (-0.60%)` | :arrow_down: |
   | [superset-frontend/src/views/CRUD/hooks.ts](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvaG9va3MudHM=) | `47.15% <0.00%> (ø)` | |
   | [...perset-frontend/src/addSlice/AddSliceContainer.tsx](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2FkZFNsaWNlL0FkZFNsaWNlQ29udGFpbmVyLnRzeA==) | `61.76% <0.00%> (ø)` | |
   | [...rontend/src/visualizations/TimeTable/TimeTable.jsx](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Zpc3VhbGl6YXRpb25zL1RpbWVUYWJsZS9UaW1lVGFibGUuanN4) | `0.00% <0.00%> (ø)` | |
   | [superset-frontend/src/utils/sortNumericValues.ts](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3V0aWxzL3NvcnROdW1lcmljVmFsdWVzLnRz) | `100.00% <0.00%> (ø)` | |
   | ... and [1 more](https://codecov.io/gh/apache/superset/pull/19071/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f9c7405...6ed0c68](https://codecov.io/gh/apache/superset/pull/19071?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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

[GitHub] [superset] diegomedina248 commented on pull request #19071: fix: Pivot Table Conditional Formatting Doesn't Show All Options

Posted by GitBox <gi...@apache.org>.
diegomedina248 commented on pull request #19071:
URL: https://github.com/apache/superset/pull/19071#issuecomment-1063109106


   @kgabryje thanks for that clarification & explanation.
   Updated the PR to address it.


-- 
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

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

Posted by GitBox <gi...@apache.org>.
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: 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