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/07/27 18:17:00 UTC

[GitHub] [superset] michael-s-molina opened a new pull request, #20891: feat: Adds drill to detail context menu for ECharts visualizations

michael-s-molina opened a new pull request, #20891:
URL: https://github.com/apache/superset/pull/20891

   ### SUMMARY
   Adds drill to detail (drill-through) context menu for ECharts visualizations. The context menu is triggered when right-clicking an EChart series. Upon selecting a drill to detail option, an alert is displayed with the selected values.
   
   The linking between the context menu and the actual feature will be handled in a future PR.
   
   The context menu was added to all ECharts with the exception of the Tree Chart.
   
   The feature is behind the `DRILL_TO_DETAIL` feature flag.
   
   This first iteration focuses on providing a minimal set of interaction possibilities for the ECharts. More advanced interactions like clicking on legends, subheaders, etc will be handled in follow-up PRs. Drilling to detail using metrics is not supported as well.
   
   @kasiazjc @lauderbaugh @rusackas @codyml 
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   https://user-images.githubusercontent.com/70410625/181340086-a6a8376f-e962-440c-b5f4-8679bc731337.mov
   
   ### TESTING INSTRUCTIONS
   
   1 - Make sure the feature does not introduce any regression when disabled. This is the default value for the feature flag.
   
   2 - Enable the `DRILL_TO_DETAIL` feature flag and check:
   - Check if there are no regressions when right-clicking the series for different charts
   - Check if the right-click works with different chart control values (dimensions, time grain, customizations, etc)
   - Check if the correct values are displayed in the context menu
   - Check if the correct values are displayed when selecting a context menu option
   - Check if other dashboard interactions are not affected by the new feature such as cross-filtering, drag-and-drop, tooltips, etc
   
   Below is the import file for the dashboard shown in the video:
   [dashboard_export_20220727T144027.zip](https://github.com/apache/superset/files/9202425/dashboard_export_20220727T144027.zip)
   
   ### ADDITIONAL INFORMATION
   - [ ] Has associated issue:
   - [x] Required feature flags: `DRILL_TO_DETAIL`
   - [x] 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
   - [ ] 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] michael-s-molina merged pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
michael-s-molina merged PR #20891:
URL: https://github.com/apache/superset/pull/20891


-- 
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] michael-s-molina commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1205249953

   /testenv up FEATURE_DASHBOARD_CROSS_FILTERS=true FEATURE_DRILL_TO_DETAIL=true


-- 
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] michael-s-molina commented on a diff in pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20891:
URL: https://github.com/apache/superset/pull/20891#discussion_r941290605


##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -172,6 +196,22 @@ class ChartRenderer extends React.Component {
     }
   }
 
+  handleOnContextMenu(filters, offsetX, offsetY) {
+    this.contextMenuRef.current.open(filters, offsetX, offsetY);
+    this.setState({ inContextMenu: true });
+  }
+
+  handleContextMenuSelected(filters) {
+    const extraFilters = this.props.formData.extra_form_data.filters || [];

Review Comment:
   Added a safe-guard πŸ‘πŸΌ 



-- 
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] geido commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
geido commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1209656834

   First of all, this looks great!
   
   A few minor issues that I found:
   
   - Not a big deal, but you can click way after the number in the Big Number
   
   <img width="1309" alt="Screenshot 2022-08-09 at 19 50 27" src="https://user-images.githubusercontent.com/60598000/183710724-8113ee02-2cb8-4bde-a296-a2da46802c0a.png">
   
   - Another small one. It would be great if the dropdown could be shown in the upper side when it does not have enough visible space to save some of the scrolling
   
   <img width="1012" alt="Screenshot 2022-08-09 at 19 59 34" src="https://user-images.githubusercontent.com/60598000/183712551-33c26e52-12a0-4b40-b5ad-d7e0e424d3a4.png">
   
   - In a graph chart we can only click on the line but not on the node, is that normal?
   
   - One more thing about the graph is that when right-clicking the animation is triggered and it creates confusion as the nodes start moving. Not sure if there is a way to stop the chart animation when right-clicking
   
   - For some reasons, when trying to close the dropdown by clicking 2 or 3 px from its left side, it won't close. I found it annoying for the user as I ended up clicking a few times to try closing it up. For some reasons, it does not happen if you click on any other side, even 1 px away from its side. This is an Antdesign issue though. Just pointing out if there is a quick fix. 
   
   I believe the above are all minor issues that we can handle in separate PRs. 
   
   
   


-- 
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] github-actions[bot] commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1200369337

   @jinghua-qa Ephemeral environment spinning up at http://34.219.213.123:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.


-- 
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] zhaoyongjie commented on a diff in pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20891:
URL: https://github.com/apache/superset/pull/20891#discussion_r941498414


##########
superset-frontend/plugins/plugin-chart-echarts/src/Radar/EchartsRadar.tsx:
##########
@@ -19,18 +19,19 @@
 import React, { useCallback } from 'react';
 import { RadarChartTransformedProps } from './types';
 import Echart from '../components/Echart';
-import { EventHandlers } from '../types';
+import { allEventHandlers } from '../utils/eventHandlers';
 
-export default function EchartsRadar({
-  height,
-  width,
-  echartOptions,
-  setDataMask,
-  labelMap,
-  groupby,
-  selectedValues,
-  formData,
-}: RadarChartTransformedProps) {
+export default function EchartsRadar(props: RadarChartTransformedProps) {
+  const {
+    height,
+    width,
+    echartOptions,
+    setDataMask,
+    labelMap,
+    groupby,
+    selectedValues,
+    formData,
+  } = props;

Review Comment:
   ~~Why does this change, for readable?~~ ignore this.



-- 
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] zhaoyongjie commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1197570625

   @michael-s-molina Looking at the screenshot, there is one thing that needs to be added. When a `time dimension` is clicked with `time grain` applied, the `time grain` needs to be passed at the same time. 
   
   BTW, why needs a `formattedVal` in the filter?


-- 
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] github-actions[bot] commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1201157949

   @michael-s-molina Ephemeral environment spinning up at http://54.148.127.0:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.


-- 
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] codyml commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1209588914

   Tested each viz type and left 2 notes for issues I found, otherwise looks great!


-- 
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] michael-s-molina commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1198113913

   @zhaoyongjie Added the time grain for `Timeseries` and `MixedTimeseries`.


-- 
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] jinghua-qa commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1200368854

   /testenv up FEATURE_DASHBOARD_CROSS_FILTERS=true FEATURE_DRILL_TO_DETAIL=true


-- 
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] github-actions[bot] commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1209616581

   @geido Ephemeral environment spinning up at http://34.216.213.189:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.


-- 
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] michael-s-molina commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1205250704

   > I found that in treemap, if i move my mouse to the white edge and it will show drill to details too, is that expected?
   
   Fixed


-- 
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] github-actions[bot] commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1199041580

   @jinghua-qa Ephemeral environment spinning up at http://54.68.14.156:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.


-- 
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] codyml commented on a diff in pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20891:
URL: https://github.com/apache/superset/pull/20891#discussion_r940605838


##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -172,6 +196,22 @@ class ChartRenderer extends React.Component {
     }
   }
 
+  handleOnContextMenu(filters, offsetX, offsetY) {
+    this.contextMenuRef.current.open(filters, offsetX, offsetY);
+    this.setState({ inContextMenu: true });
+  }
+
+  handleContextMenuSelected(filters) {
+    const extraFilters = this.props.formData.extra_form_data.filters || [];

Review Comment:
   I'm getting an error on this line when I try to select "Drill by x": `Cannot read properties of undefined (reading 'filters')`.  I think it might be because I'm running it with `DRILL_TO_DETAIL` only and not `DASHBOARD_CROSS_FILTERS`, and it seems like the `extra_form_filters` isn't initialized as an empty object when `DASHBOARD_CROSS_FILTERS` isn't enabled.



-- 
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] zhaoyongjie commented on a diff in pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20891:
URL: https://github.com/apache/superset/pull/20891#discussion_r941496485


##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -92,13 +113,16 @@ class ChartRenderer extends React.Component {
     };
   }
 
-  shouldComponentUpdate(nextProps) {
+  shouldComponentUpdate(nextProps, nextState) {
     const resultsReady =
       nextProps.queriesResponse &&
       ['success', 'rendered'].indexOf(nextProps.chartStatus) > -1 &&
       !nextProps.queriesResponse?.[0]?.error;
 
     if (resultsReady) {
+      if (!isEqual(this.state, nextState)) {
+        return true;
+      }

Review Comment:
   ```suggestion
        return !isEqual(this.state, nextState)
   ```



##########
superset-frontend/plugins/plugin-chart-echarts/src/Radar/EchartsRadar.tsx:
##########
@@ -19,18 +19,19 @@
 import React, { useCallback } from 'react';
 import { RadarChartTransformedProps } from './types';
 import Echart from '../components/Echart';
-import { EventHandlers } from '../types';
+import { allEventHandlers } from '../utils/eventHandlers';
 
-export default function EchartsRadar({
-  height,
-  width,
-  echartOptions,
-  setDataMask,
-  labelMap,
-  groupby,
-  selectedValues,
-  formData,
-}: RadarChartTransformedProps) {
+export default function EchartsRadar(props: RadarChartTransformedProps) {
+  const {
+    height,
+    width,
+    echartOptions,
+    setDataMask,
+    labelMap,
+    groupby,
+    selectedValues,
+    formData,
+  } = props;

Review Comment:
   Why does this change, for readable?



-- 
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] michael-s-molina commented on a diff in pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20891:
URL: https://github.com/apache/superset/pull/20891#discussion_r941648346


##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -92,13 +113,16 @@ class ChartRenderer extends React.Component {
     };
   }
 
-  shouldComponentUpdate(nextProps) {
+  shouldComponentUpdate(nextProps, nextState) {
     const resultsReady =
       nextProps.queriesResponse &&
       ['success', 'rendered'].indexOf(nextProps.chartStatus) > -1 &&
       !nextProps.queriesResponse?.[0]?.error;
 
     if (resultsReady) {
+      if (!isEqual(this.state, nextState)) {
+        return true;
+      }

Review Comment:
   I don't want to return unless the state changes. If the state is the same, it should execute the code below these lines. 
   
   ```
   if (!isEqual(this.state, nextState)) {
      return true;
   }
   
   this.hasQueryResponseChange = nextProps.queriesResponse !== this.props.queriesResponse;
   ...
   ```



-- 
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] codyml commented on a diff in pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20891:
URL: https://github.com/apache/superset/pull/20891#discussion_r940616664


##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberViz.tsx:
##########
@@ -159,13 +165,25 @@ class BigNumberVis extends React.PureComponent<BigNumberVisProps> {
     });
     container.remove();
 
+    const onContextMenu = (e: MouseEvent<HTMLDivElement>) => {
+      if (this.props.onContextMenu) {
+        e.preventDefault();
+        this.props.onContextMenu(
+          [],

Review Comment:
   For this one, even though you can't drill to detail based on the trendline because the timestamp is not a dimension (I'm assuming?), it was a little confusing for me to not be able to right-click on the trendline at all.  What would you think about adding a plain "Drill to detail" menu option that doesn't add any filters when you right-click on the trendline?



-- 
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] github-actions[bot] commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1209818808

   Ephemeral environment shutdown and build artifacts deleted.


-- 
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] geido commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
geido commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1209597559

   /testenv up FEATURE_DASHBOARD_CROSS_FILTERS=true FEATURE_DRILL_TO_DETAIL=true
   
   


-- 
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] michael-s-molina commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1209721242

   @codyml @geido Great comments! Thanks again for testing the feature.
   
   My last commit fixed:
   > If you right-click on the dimension label for Treemap v2, the menu looks like it has a broken option:
   
   > Not a big deal, but you can click way after the number in the Big Number
   
   I decided to follow @geido's suggestion and tackle the remaining comments in follow-up PRs so that @codyml can rebase his PR on top of this one. I'll create tickets for them.


-- 
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] michael-s-molina commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1201153794

   /testenv up FEATURE_DASHBOARD_CROSS_FILTERS=true FEATURE_DRILL_TO_DETAIL=true


-- 
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] github-actions[bot] commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1205255392

   @michael-s-molina Ephemeral environment spinning up at http://35.89.204.156:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.


-- 
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] codyml commented on a diff in pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20891:
URL: https://github.com/apache/superset/pull/20891#discussion_r941486292


##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberViz.tsx:
##########
@@ -62,6 +64,13 @@ type BigNumberVisProps = {
   trendLineData?: TimeSeriesDatum[];
   mainColor: string;
   echartOptions: EChartsCoreOption;
+  onContextMenu?: (
+    filters: QueryObjectFilterClause[],
+    offsetX: number,

Review Comment:
   I'm seeing some strange tooltip positioning for Big Number and Big Number w/ Trendline:
   <img width="311" alt="Big Number" src="https://user-images.githubusercontent.com/13007381/183691452-d262bc2f-4856-4ebc-9580-4913fec9c60e.png">
   
   <img width="224" alt="Big Number w: Trendline" src="https://user-images.githubusercontent.com/13007381/183691605-3c3ea8e2-e7a0-4ea9-8d61-691b59a4a742.png">
   
   Pie chart for reference:
   <img width="438" alt="Pie" src="https://user-images.githubusercontent.com/13007381/183691815-926755eb-6e5b-4cd0-8b5e-88c2e75a0cfa.png">
   
   



-- 
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] michael-s-molina commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1198001327

   > @michael-s-molina Looking at the screenshot, there is one thing that needs to be added. When a `time dimension` is clicked with `time grain` applied, the `time grain` needs to be passed at the same time.
   
   Nice point! I'll work on it.
   
   > BTW, why needs a `formattedVal` in the filter?
   
   Currently, the logic for formatting the values is contained in each plugin. Depending on the plugin type the values can be formatted differently. When exhibiting these values in the context menu and also later in the modal, we need to present the values with the exact formatting used by the plugin. At the same time, we need the original value for the endpoint. Some examples are dates, currencies, decimals, etc.


-- 
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] jinghua-qa commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1198999921

   /testenv up


-- 
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] codyml commented on a diff in pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20891:
URL: https://github.com/apache/superset/pull/20891#discussion_r941517581


##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/EchartsTreemap.tsx:
##########
@@ -84,6 +86,25 @@ export default function EchartsTreemap({
         handleChange([name]);
       }
     },
+    contextmenu: eventParams => {

Review Comment:
   If you right-click on the dimension label for Treemap v2, the menu looks like it has a broken option:
   
   <img width="353" alt="Screen Shot 2022-08-09 at 9 48 54 AM" src="https://user-images.githubusercontent.com/13007381/183699108-fa4b3539-05fe-4cbb-91ed-584241ba92d8.png">
   
   



-- 
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 #20891: feat: Adds drill to detail context menu for ECharts visualizations

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

   # [Codecov](https://codecov.io/gh/apache/superset/pull/20891?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 [#20891](https://codecov.io/gh/apache/superset/pull/20891?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4bd2c82) into [master](https://codecov.io/gh/apache/superset/commit/383313b105b0e82bea0f38cc971630eded5affe0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (383313b) will **decrease** coverage by `0.72%`.
   > The diff coverage is `26.58%`.
   
   > :exclamation: Current head 4bd2c82 differs from pull request most recent head 7c0184a. Consider uploading reports for the commit 7c0184a to get more accurate results
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #20891      +/-   ##
   ==========================================
   - Coverage   66.27%   65.55%   -0.73%     
   ==========================================
     Files        1758     1758              
     Lines       67072    67072              
     Branches     7122     7122              
   ==========================================
   - Hits        44453    43969     -484     
   - Misses      20800    21284     +484     
     Partials     1819     1819              
   ```
   
   | Flag | Coverage Ξ” | |
   |---|---|---|
   | hive | `53.25% <ΓΈ> (ΓΈ)` | |
   | mysql | `?` | |
   | postgres | `?` | |
   | presto | `53.15% <ΓΈ> (ΓΈ)` | |
   | python | `80.06% <ΓΈ> (-1.50%)` | :arrow_down: |
   | sqlite | `79.67% <ΓΈ> (ΓΈ)` | |
   | unit | `?` | |
   
   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/20891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Ξ” | |
   |---|---|---|
   | [...packages/superset-ui-core/src/query/types/Query.ts](https://codecov.io/gh/apache/superset/pull/20891/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvUXVlcnkudHM=) | `100.00% <ΓΈ> (ΓΈ)` | |
   | [...ackages/superset-ui-core/src/utils/featureFlags.ts](https://codecov.io/gh/apache/superset/pull/20891/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdXRpbHMvZmVhdHVyZUZsYWdzLnRz) | `100.00% <ΓΈ> (ΓΈ)` | |
   | [...rts/src/BigNumber/BigNumberTotal/transformProps.ts](https://codecov.io/gh/apache/superset/pull/20891/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQmlnTnVtYmVyL0JpZ051bWJlclRvdGFsL3RyYW5zZm9ybVByb3BzLnRz) | `0.00% <0.00%> (ΓΈ)` | |
   | [...lugin-chart-echarts/src/BigNumber/BigNumberViz.tsx](https://codecov.io/gh/apache/superset/pull/20891/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQmlnTnVtYmVyL0JpZ051bWJlclZpei50c3g=) | `0.00% <0.00%> (ΓΈ)` | |
   | [...lugin-chart-echarts/src/BoxPlot/EchartsBoxPlot.tsx](https://codecov.io/gh/apache/superset/pull/20891/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQm94UGxvdC9FY2hhcnRzQm94UGxvdC50c3g=) | `0.00% <0.00%> (ΓΈ)` | |
   | [.../plugins/plugin-chart-echarts/src/BoxPlot/types.ts](https://codecov.io/gh/apache/superset/pull/20891/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQm94UGxvdC90eXBlcy50cw==) | `0.00% <ΓΈ> (ΓΈ)` | |
   | [.../plugin-chart-echarts/src/Funnel/EchartsFunnel.tsx](https://codecov.io/gh/apache/superset/pull/20891/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvRnVubmVsL0VjaGFydHNGdW5uZWwudHN4) | `0.00% <0.00%> (ΓΈ)` | |
   | [...d/plugins/plugin-chart-echarts/src/Funnel/types.ts](https://codecov.io/gh/apache/superset/pull/20891/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvRnVubmVsL3R5cGVzLnRz) | `100.00% <ΓΈ> (ΓΈ)` | |
   | [...ns/plugin-chart-echarts/src/Gauge/EchartsGauge.tsx](https://codecov.io/gh/apache/superset/pull/20891/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvR2F1Z2UvRWNoYXJ0c0dhdWdlLnRzeA==) | `0.00% <0.00%> (ΓΈ)` | |
   | [...ns/plugin-chart-echarts/src/Graph/EchartsGraph.tsx](https://codecov.io/gh/apache/superset/pull/20891/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvR3JhcGgvRWNoYXJ0c0dyYXBoLnRzeA==) | `0.00% <0.00%> (ΓΈ)` | |
   | ... and [91 more](https://codecov.io/gh/apache/superset/pull/20891/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) | |
   
   Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?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] jinghua-qa commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1202962413

   I found that in treemap, if i move my mouse to the white edge and it will show drill to details too, is that expected?
   
   https://user-images.githubusercontent.com/81597121/182424965-323f2549-47f6-4802-b09b-9af455c0f35d.mov
   
   


-- 
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] codyml commented on a diff in pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20891:
URL: https://github.com/apache/superset/pull/20891#discussion_r940605838


##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -172,6 +196,22 @@ class ChartRenderer extends React.Component {
     }
   }
 
+  handleOnContextMenu(filters, offsetX, offsetY) {
+    this.contextMenuRef.current.open(filters, offsetX, offsetY);
+    this.setState({ inContextMenu: true });
+  }
+
+  handleContextMenuSelected(filters) {
+    const extraFilters = this.props.formData.extra_form_data.filters || [];

Review Comment:
   I'm getting an error on this line when I try to select "Drill by x": `Cannot read properties of undefined (reading 'filters')`.  I think it might be because I'm running it with `DRILL_TO_DETAIL` only and not `DASHBOARD_CROSS_FILTERS`, and it seems like the `extra_form_data` isn't initialized as an empty object when `DASHBOARD_CROSS_FILTERS` isn't enabled.



-- 
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] michael-s-molina commented on pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20891:
URL: https://github.com/apache/superset/pull/20891#issuecomment-1202982641

   > I found that in treemap, if i move my mouse to the white edge and it will show drill to details too, is that expected?
   
   No, it's not πŸ˜†. I'll fix 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] michael-s-molina commented on a diff in pull request #20891: feat: Adds drill to detail context menu for ECharts visualizations

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20891:
URL: https://github.com/apache/superset/pull/20891#discussion_r941290605


##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -172,6 +196,22 @@ class ChartRenderer extends React.Component {
     }
   }
 
+  handleOnContextMenu(filters, offsetX, offsetY) {
+    this.contextMenuRef.current.open(filters, offsetX, offsetY);
+    this.setState({ inContextMenu: true });
+  }
+
+  handleContextMenuSelected(filters) {
+    const extraFilters = this.props.formData.extra_form_data.filters || [];

Review Comment:
   Added a safeguard πŸ‘πŸΌ 



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