You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "kgabryje (via GitHub)" <gi...@apache.org> on 2023/04/12 15:04:27 UTC

[GitHub] [superset] kgabryje opened a new pull request, #23664: feat: Implement breadcrumbs in Drill By modal

kgabryje opened a new pull request, #23664:
URL: https://github.com/apache/superset/pull/23664

   ### SUMMARY
   This PR implements breadcrumbs component in the Drill By modal in order to:
   1. Show the history of drillings (currently applied groupings and filters)
   2. Allow user to undo drilling actions
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   
   https://user-images.githubusercontent.com/15073128/231499232-10a47d5b-ef0f-4b6a-9a65-ad28ee38e576.mov
   
   
   
   ### TESTING INSTRUCTIONS
   0. Enable `DRILL_BY` feature flag
   1. Perform drill by action
   2. Verify that breadcrumbs display the history of drilling - in the format `groupby1 (filter1) / groupby2 (filter2) / current_groupby`
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [x] Required feature flags: `DRILL_BY`
   - [ ] 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
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   CC @kasiazjc


-- 
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] villebro commented on a diff in pull request #23664: feat: Implement breadcrumbs in Drill By modal

Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #23664:
URL: https://github.com/apache/superset/pull/23664#discussion_r1164367974


##########
superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx:
##########
@@ -116,36 +122,87 @@ export default function DrillByModal({
   filters,
   formData,
   groupbyFieldName = 'groupby',
+  adhocFilterFieldName = 'adhoc_filters',
   onHideModal,
 }: DrillByModalProps) {
   const theme = useTheme();
-  const [chartDataResult, setChartDataResult] = useState<QueryData[]>();
-  const [drillByDisplayMode, setDrillByDisplayMode] = useState<DrillByType>(
-    DrillByType.Chart,
+
+  const initialGroupbyColumns = useMemo(
+    () =>
+      ensureIsArray(formData[groupbyFieldName])
+        .map(colName =>
+          dataset.columns?.find(col => col.column_name === colName),
+        )
+        .filter(isDefined),
+    [dataset.columns, formData, groupbyFieldName],
   );
+
+  const { displayModeToggle, drillByDisplayMode } = useDisplayModeToggle();
+  const [chartDataResult, setChartDataResult] = useState<QueryData[]>();
   const [datasourceId] = useMemo(
     () => formData.datasource.split('__'),
     [formData.datasource],
   );
 
-  const [currentColumn, setCurrentColumn] = useState(column);
+  // currentColumn can be an array when the original chart is grouped by multiple
+  // columns and user navigates back to the original chart by clicking the first
+  // breadcrumb
+  const [currentColumn, setCurrentColumn] = useState<
+    Column | Column[] | undefined
+  >(column);

Review Comment:
   Just a thought - would it potentially simplify the code, if we'd do something like this (we'd also do something similar in those other similar code blocks):
   ```typescript
     const [currentColumns, setCurrentColumns] = useState(ensureIsArray(column));
   ```
   The we wouldn't have to jump between `currentColumn` and `currentColumns` or worry about it potentially being `undefined`.



-- 
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] villebro commented on a diff in pull request #23664: feat: Implement breadcrumbs in Drill By modal

Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #23664:
URL: https://github.com/apache/superset/pull/23664#discussion_r1164367974


##########
superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx:
##########
@@ -116,36 +122,87 @@ export default function DrillByModal({
   filters,
   formData,
   groupbyFieldName = 'groupby',
+  adhocFilterFieldName = 'adhoc_filters',
   onHideModal,
 }: DrillByModalProps) {
   const theme = useTheme();
-  const [chartDataResult, setChartDataResult] = useState<QueryData[]>();
-  const [drillByDisplayMode, setDrillByDisplayMode] = useState<DrillByType>(
-    DrillByType.Chart,
+
+  const initialGroupbyColumns = useMemo(
+    () =>
+      ensureIsArray(formData[groupbyFieldName])
+        .map(colName =>
+          dataset.columns?.find(col => col.column_name === colName),
+        )
+        .filter(isDefined),
+    [dataset.columns, formData, groupbyFieldName],
   );
+
+  const { displayModeToggle, drillByDisplayMode } = useDisplayModeToggle();
+  const [chartDataResult, setChartDataResult] = useState<QueryData[]>();
   const [datasourceId] = useMemo(
     () => formData.datasource.split('__'),
     [formData.datasource],
   );
 
-  const [currentColumn, setCurrentColumn] = useState(column);
+  // currentColumn can be an array when the original chart is grouped by multiple
+  // columns and user navigates back to the original chart by clicking the first
+  // breadcrumb
+  const [currentColumn, setCurrentColumn] = useState<
+    Column | Column[] | undefined
+  >(column);

Review Comment:
   Just a thought - would it potentially simplify the code, if we'd do something like this (we'd also do something similar in those other similar code blocks):
   ```typescript
     const [currentColumns, setCurrentColumns] = useState(ensureIsArray(column));
   ```
   Then we wouldn't have to jump between `currentColumn` and `currentColumns` or worry about it potentially being `undefined`.



-- 
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 merged pull request #23664: feat: Implement breadcrumbs in Drill By modal

Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje merged PR #23664:
URL: https://github.com/apache/superset/pull/23664


-- 
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 #23664: feat: Implement breadcrumbs in Drill By modal

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23664:
URL: https://github.com/apache/superset/pull/23664#issuecomment-1505535303

   ## [Codecov](https://codecov.io/gh/apache/superset/pull/23664?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 [#23664](https://codecov.io/gh/apache/superset/pull/23664?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (815f05a) into [master](https://codecov.io/gh/apache/superset/commit/587e7759b1b674440ac0aa705ebae6599564875f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (587e775) will **decrease** coverage by `0.03%`.
   > The diff coverage is `72.41%`.
   
   > :exclamation: Current head 815f05a differs from pull request most recent head 61c2be2. Consider uploading reports for the commit 61c2be2 to get more accurate results
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #23664      +/-   ##
   ==========================================
   - Coverage   68.02%   68.00%   -0.03%     
   ==========================================
     Files        1919     1921       +2     
     Lines       73932    74027      +95     
     Branches     8067     8096      +29     
   ==========================================
   + Hits        50291    50339      +48     
   - Misses      21574    21615      +41     
   - Partials     2067     2073       +6     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `54.11% <72.41%> (+0.04%)` | :arrow_up: |
   | sqlite | `?` | |
   
   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/23664?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/23664?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% <ø> (ø)` | |
   | [...s/plugin-chart-echarts/src/Timeseries/constants.ts](https://codecov.io/gh/apache/superset/pull/23664?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy9jb25zdGFudHMudHM=) | `100.00% <ø> (ø)` | |
   | [...gin-chart-echarts/src/Timeseries/transformProps.ts](https://codecov.io/gh/apache/superset/pull/23664?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy90cmFuc2Zvcm1Qcm9wcy50cw==) | `57.27% <0.00%> (-1.07%)` | :arrow_down: |
   | [...tend/plugins/plugin-chart-echarts/src/constants.ts](https://codecov.io/gh/apache/superset/pull/23664?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvY29uc3RhbnRzLnRz) | `100.00% <ø> (ø)` | |
   | [...tend/plugins/plugin-chart-echarts/src/controls.tsx](https://codecov.io/gh/apache/superset/pull/23664?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvY29udHJvbHMudHN4) | `76.00% <ø> (ø)` | |
   | [...onents/Chart/ChartContextMenu/ChartContextMenu.tsx](https://codecov.io/gh/apache/superset/pull/23664?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvQ2hhcnRDb250ZXh0TWVudS9DaGFydENvbnRleHRNZW51LnRzeA==) | `76.19% <ø> (ø)` | |
   | [...rt-controls/src/shared-controls/customControls.tsx](https://codecov.io/gh/apache/superset/pull/23664?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9jdXN0b21Db250cm9scy50c3g=) | `24.13% <50.00%> (+6.74%)` | :arrow_up: |
   | [...tend/src/components/Chart/DrillBy/DrillByModal.tsx](https://codecov.io/gh/apache/superset/pull/23664?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvRHJpbGxCeS9EcmlsbEJ5TW9kYWwudHN4) | `72.09% <57.14%> (-10.96%)` | :arrow_down: |
   | [...d/plugins/plugin-chart-echarts/src/utils/series.ts](https://codecov.io/gh/apache/superset/pull/23664?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvdXRpbHMvc2VyaWVzLnRz) | `86.62% <80.00%> (-1.87%)` | :arrow_down: |
   | [...components/Chart/DrillBy/useDrillByBreadcrumbs.tsx](https://codecov.io/gh/apache/superset/pull/23664?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvRHJpbGxCeS91c2VEcmlsbEJ5QnJlYWRjcnVtYnMudHN4) | `87.50% <87.50%> (ø)` | |
   | ... and [4 more](https://codecov.io/gh/apache/superset/pull/23664?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ... and [5 files with indirect coverage changes](https://codecov.io/gh/apache/superset/pull/23664/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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] villebro commented on a diff in pull request #23664: feat: Implement breadcrumbs in Drill By modal

Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #23664:
URL: https://github.com/apache/superset/pull/23664#discussion_r1165589070


##########
superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx:
##########
@@ -127,7 +127,6 @@ export default function DrillByModal({
 }: DrillByModalProps) {
   const theme = useTheme();
 
-  console.log('DUPA', formData);

Review Comment:
   ![image](https://user-images.githubusercontent.com/33317356/231785658-ab4ea895-7ec8-4100-bc7b-6f09c2e5d691.png)
   Not like I've ever done anything similar 😆😆😆



-- 
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 #23664: feat: Implement breadcrumbs in Drill By modal

Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #23664:
URL: https://github.com/apache/superset/pull/23664#issuecomment-1507008634

   @villebro @lilykuang Thanks for comments. I decided to do a bit of refactoring so that `currentColumn` can only be `Column | undefined`, not an array, as was originally intended. I think the code and flow is simpler now since we eliminated a special case which would only occur if user clicked on the first breadcrumb of a chart that was initially grouped by 2 columns.
   Also, added unit tests for breadcrumbs


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