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/11/09 15:24:26 UTC

[GitHub] [superset] michael-s-molina opened a new pull request, #22082: fix: Drill to detail blocked by tooltip

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

   ### SUMMARY
   Changes the default value of `tooltip.confine` property to `false` which is the default value in ECharts. The objective is to prevent the tooltip to block the mouse pointer and allow further operations such as drill to detail.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   https://user-images.githubusercontent.com/70410625/200870026-c792045b-d0d6-4224-8661-19ed9ea54834.mov
   
   https://user-images.githubusercontent.com/70410625/200870121-ec961baf-2a53-4824-8bf8-37e4affbdbce.mov
   
   ### TESTING INSTRUCTIONS
   Check the videos for instructions.
   
   ### 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
   - [ ] 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 commented on pull request #22082: fix: Drill to detail blocked by tooltip

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

   > I left a long reply to your comment, but TL;DR, even if we don't implement an exotic position callback, I think your solution might be better than the current behavior, which isn't completely unproblematic, either. So I'm approving this, but it could be fun to try to create a better positioning function and maybe contribute that upstream? Happy to pair on it if you're interested 🙂
   
   Looking at the examples you posted I'm still not happy with the final result. I agree we should try to improve the algorithm. Did you see this [one](https://github.com/apache/echarts/issues/5004#issuecomment-559668309)? I'll leave this open so we can collaborate.


-- 
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 #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/types.ts:
##########
@@ -16,20 +16,22 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+import { QueryFormData } from '@superset-ui/core';
 import {
-  ChartDataResponseResult,
-  ChartProps,
-  QueryFormData,
-} from '@superset-ui/core';
-import { EchartsTitleFormData, EChartTransformedProps } from '../types';
+  BaseChartProps,
+  BaseTransformedProps,
+  ContextMenuTransformedProps,
+  CrossFilterTransformedProps,
+  TitleFormData,
+} from '../types';
 import { DEFAULT_TITLE_FORM_DATA } from '../constants';
 
 export type BoxPlotQueryFormData = QueryFormData & {
   numberFormat?: string;
   whiskerOptions?: BoxPlotFormDataWhiskerOptions;
   xTickLayout?: BoxPlotFormXTickLayout;
   emitFilter: boolean;
-} & EchartsTitleFormData;
+} & TitleFormData;

Review Comment:
   I decided to remove the `Echarts` prefix from some types as it felt slightly redundant (this is an ECharts plugin, after all)



-- 
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 #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -48,9 +48,12 @@ export default function EchartsTimeseries({
   onContextMenu,
   xValueFormatter,
   xAxis,
+  refs,
 }: TimeseriesChartTransformedProps) {
   const { emitFilter, stack } = formData;
   const echartRef = useRef<EchartsHandler | null>(null);
+  // eslint-disable-next-line no-param-reassign
+  refs.echartRef = echartRef;

Review Comment:
   It was previously being passed to the `Echart` component in a separate dedicated `ref` field. To avoid having two properties (one for the div ref and one for the echart ref), I decided to just place both of them in that `refs` object (I was becoming tired at this point and running low on imagination 😄 )



-- 
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 #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -48,9 +48,12 @@ export default function EchartsTimeseries({
   onContextMenu,
   xValueFormatter,
   xAxis,
+  refs,
 }: TimeseriesChartTransformedProps) {
   const { emitFilter, stack } = formData;
   const echartRef = useRef<EchartsHandler | null>(null);
+  // eslint-disable-next-line no-param-reassign
+  refs.echartRef = echartRef;

Review Comment:
   It was passed to `Echart` in a separate dedicated `ref` field. TO avoid having two properties (one for the div ref and one for the echart ref), I decided to just place both of them in that `refs` object (I was becoming tired at this point and running low on imagination 😄 )



-- 
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 #22082: fix: Drill to detail blocked by tooltip

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

   @villebro Ephemeral environment spinning up at http://34.220.125.160: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] villebro commented on a diff in pull request #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/defaults.ts:
##########
@@ -23,7 +23,7 @@ export const defaultGrid = {
 };
 
 export const defaultTooltip = {
-  confine: true,
+  confine: false,

Review Comment:
   IIRC, we originally added this to make sure the tooltip doesn't leave the window. See https://github.com/apache/echarts/issues/5004. However, this does have the drawback of being suboptimal if the chart is small and the tooltip is big. It would be nice if ECharts automatically made sure the tooltip isn't cut, but in the interim, maybe we could disable `confine` as suggested and use a custom position callback to make sure it's within the window's boundaries but doesn't overlap the pointer? 
   ```
   position: function(pt) {
     const x = <some logic based on window width etc>;
     const y = <some logic based on window height etc>;
     return [x, y];
   },
   ```
   
   Check the example here by clicking "Try it" to experiment: https://echarts.apache.org/en/option.html#tooltip.confine
   
   <img width="1318" alt="image" src="https://user-images.githubusercontent.com/33317356/201314242-07a916fc-328f-4e7b-b67c-46ff09511a8f.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 a diff in pull request #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -48,9 +48,12 @@ export default function EchartsTimeseries({
   onContextMenu,
   xValueFormatter,
   xAxis,
+  refs,
 }: TimeseriesChartTransformedProps) {
   const { emitFilter, stack } = formData;
   const echartRef = useRef<EchartsHandler | null>(null);
+  // eslint-disable-next-line no-param-reassign
+  refs.echartRef = echartRef;

Review Comment:
   It seems `echartRef` is only being used internally in `EchartsTimeseries`. Do we need to add it to `refs`?



-- 
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 #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/Gauge/types.ts:
##########
@@ -80,10 +80,11 @@ export const DEFAULT_FORM_DATA: Partial<EchartsGaugeFormData> = {
 };
 
 export interface EchartsGaugeChartProps
-  extends ChartProps<EchartsGaugeFormData> {
+  extends BaseChartProps<EchartsGaugeFormData> {
   formData: EchartsGaugeFormData;
-  queriesData: ChartDataResponseResult[];
 }
 
 export type GaugeChartTransformedProps =
-  EChartTransformedProps<EchartsGaugeFormData>;
+  BaseTransformedProps<EchartsGaugeFormData> &
+    ContextMenuTransformedProps &
+    CrossFilterTransformedProps;

Review Comment:
   I decided to split out the context menu and cross filtering props into separate types to avoid code duplication.



-- 
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 #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/tooltip.ts:
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { CallbackDataParams } from 'echarts/types/src/util/types';
+import { TOOLTIP_OVERFLOW_MARGIN, TOOLTIP_POINTER_MARGIN } from '../constants';
+import { Refs } from '../types';
+
+export function getDefaultPosition(refs: Refs) {
+  return (
+    canvasMousePos: [number, number],
+    params: CallbackDataParams,
+    tooltipDom: HTMLDivElement | null,
+    rect: any,

Review Comment:
   ```suggestion
   ```



##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/tooltip.ts:
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { CallbackDataParams } from 'echarts/types/src/util/types';

Review Comment:
   ```suggestion
   ```



-- 
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 #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/defaults.ts:
##########
@@ -23,7 +23,7 @@ export const defaultGrid = {
 };
 
 export const defaultTooltip = {
-  confine: true,
+  confine: false,

Review Comment:
   Thanks for the context @villebro. What I noticed is that `confine` is used when overriding the positioning function. I wasn't able to make the tooltip overflow when using the default positioning function with `confine: false`.
   
   https://user-images.githubusercontent.com/70410625/201763499-9ce58788-ef88-460d-8e57-20f871195083.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] michael-s-molina commented on a diff in pull request #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/tooltip.ts:
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { CallbackDataParams } from 'echarts/types/src/util/types';
+import { TOOLTIP_OVERFLOW_MARGIN, TOOLTIP_POINTER_MARGIN } from '../constants';
+import { Refs } from '../types';
+
+export function getDefaultPosition(refs: Refs) {
+  return (
+    canvasMousePos: [number, number],
+    params: CallbackDataParams,
+    tooltipDom: HTMLDivElement | null,
+    rect: any,

Review Comment:
   ```suggestion
   ```



##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/tooltip.ts:
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { CallbackDataParams } from 'echarts/types/src/util/types';

Review Comment:
   ```suggestion
   ```



-- 
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 #22082: fix: Drill to detail blocked by tooltip

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

   @villebro and @kgabryje I added you as reviewers because this PR is changing the code that you wrote and you may have additional context.


-- 
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 #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/transformProps.ts:
##########
@@ -38,6 +41,7 @@ export default function transformProps(chartProps: BigNumberTotalChartProps) {
     timeFormat,
     yAxisFormat,
   } = formData;
+  const refs: Refs = {};

Review Comment:
   the `refs` object is needed here so the downstream code (`transformProps` -> Chart plugin component -> `Echart` component) can add the reference to the chart div for the tooltip positioning callback to calculate the current location of the pointer.



-- 
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 #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/transformProps.ts:
##########
@@ -38,6 +41,7 @@ export default function transformProps(chartProps: BigNumberTotalChartProps) {
     timeFormat,
     yAxisFormat,
   } = formData;
+  const refs: Refs = {};

Review Comment:
   the `refs` object is needed here so the downstream code can add the reference to the chart div for the tooltip positioning callback to calculate the current location of the pointer.



-- 
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 #22082: fix: Drill to detail blocked by tooltip

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

   /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] villebro commented on pull request #22082: fix: Drill to detail blocked by tooltip

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

   /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] michael-s-molina commented on a diff in pull request #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -48,9 +48,12 @@ export default function EchartsTimeseries({
   onContextMenu,
   xValueFormatter,
   xAxis,
+  refs,
 }: TimeseriesChartTransformedProps) {
   const { emitFilter, stack } = formData;
   const echartRef = useRef<EchartsHandler | null>(null);
+  // eslint-disable-next-line no-param-reassign
+  refs.echartRef = echartRef;

Review Comment:
   Bu `Echart` is not using `echartRef`



##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -48,9 +48,12 @@ export default function EchartsTimeseries({
   onContextMenu,
   xValueFormatter,
   xAxis,
+  refs,
 }: TimeseriesChartTransformedProps) {
   const { emitFilter, stack } = formData;
   const echartRef = useRef<EchartsHandler | null>(null);
+  // eslint-disable-next-line no-param-reassign
+  refs.echartRef = echartRef;

Review Comment:
   `Echart` is not using `echartRef`



-- 
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 #22082: fix: Drill to detail blocked by tooltip

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

   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] codecov[bot] commented on pull request #22082: fix: Drill to detail blocked by tooltip

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

   # [Codecov](https://codecov.io/gh/apache/superset/pull/22082?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 [#22082](https://codecov.io/gh/apache/superset/pull/22082?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bf5cd42) into [master](https://codecov.io/gh/apache/superset/commit/4496748cd942b037a9e497edc336bd3e3d48717a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4496748) will **not change** coverage.
   > The diff coverage is `n/a`.
   
   ```diff
   @@           Coverage Diff           @@
   ##           master   #22082   +/-   ##
   =======================================
     Coverage   67.12%   67.12%           
   =======================================
     Files        1818     1818           
     Lines       69639    69639           
     Branches     7496     7496           
   =======================================
     Hits        46747    46747           
     Misses      20954    20954           
     Partials     1938     1938           
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `53.70% <ø> (ø)` | |
   
   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/22082?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...BigNumber/BigNumberWithTrendline/transformProps.ts](https://codecov.io/gh/apache/superset/pull/22082/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQmlnTnVtYmVyL0JpZ051bWJlcldpdGhUcmVuZGxpbmUvdHJhbnNmb3JtUHJvcHMudHM=) | `47.05% <ø> (ø)` | |
   | [...ntend/plugins/plugin-chart-echarts/src/defaults.ts](https://codecov.io/gh/apache/superset/pull/22082/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvZGVmYXVsdHMudHM=) | `100.00% <ø> (ø)` | |
   
   :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 #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberViz.tsx:
##########
@@ -311,7 +291,7 @@ class BigNumberVis extends React.PureComponent<BigNumberVisProps> {
     return (
       <div className={className} style={{ height }}>
         {this.renderFallbackWarning()}
-        {this.renderKicker(kickerFontSize * height)}
+        {this.renderKicker((kickerFontSize || 0) * height)}

Review Comment:
   lots of minor checks that needed to be added when the interfaces/types where updated



-- 
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 #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:
##########
@@ -106,6 +111,7 @@ function Echart(
   // did mount
   useEffect(() => {
     handleSizeChange({ width, height });
+    return () => chartRef.current?.dispose();

Review Comment:
   This fixes the sticky tooltip bug (disposes the chart instance during cleanup)



-- 
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 merged pull request #22082: fix: Drill to detail blocked by tooltip

Posted by GitBox <gi...@apache.org>.
villebro merged PR #22082:
URL: https://github.com/apache/superset/pull/22082


-- 
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 #22082: fix: Drill to detail blocked by tooltip

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

   @michael-s-molina Ephemeral environment spinning up at http://54.188.153.91: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] kgabryje commented on pull request #22082: fix: Drill to detail blocked by tooltip

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

   I looked through the code and tested the flows on eph env. Looks great to me!


-- 
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 #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/types.ts:
##########
@@ -43,15 +48,50 @@ export type BigNumberWithTrendlineFormData = BigNumberTotalFormData & {
   compareLag?: string | number;
 };
 
-export type BigNumberTotalChartProps = ChartProps<QueryFormData> & {
-  formData: BigNumberTotalFormData;
-  queriesData: (ChartDataResponseResult & {
-    data?: BigNumberDatum[];
-  })[];
-};
+export interface BigNumberTotalChartDataResponseResult
+  extends ChartDataResponseResult {
+  data: BigNumberDatum[];
+}
 
-export type BigNumberWithTrendlineChartProps = BigNumberTotalChartProps & {
-  formData: BigNumberWithTrendlineFormData;
-};
+export type BigNumberTotalChartProps =
+  BaseChartProps<BigNumberTotalFormData> & {
+    formData: BigNumberTotalFormData;
+    queriesData: BigNumberTotalChartDataResponseResult[];
+  };
+
+export type BigNumberWithTrendlineChartProps =
+  BaseChartProps<BigNumberWithTrendlineFormData> & {
+    formData: BigNumberWithTrendlineFormData;
+  };
 
 export type TimeSeriesDatum = [number, number | null];
+
+export type BigNumberVizProps = {

Review Comment:
   note: This whole chart should be refactored to be a functional component



-- 
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 #22082: fix: Drill to detail blocked by tooltip

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


##########
superset-frontend/plugins/plugin-chart-echarts/src/defaults.ts:
##########
@@ -23,7 +23,7 @@ export const defaultGrid = {
 };
 
 export const defaultTooltip = {
-  confine: true,
+  confine: false,

Review Comment:
   Oh ok, I hadn't noticed that. I dug out the original PR that enabled confine: https://github.com/apache-superset/superset-ui/pull/764 Here's a screencapture of way too many items on the tooltip with `confine: true`:
   
   https://user-images.githubusercontent.com/33317356/202183731-fa6e8310-5f25-4853-83a0-b30c2991c58a.mp4
   
   And the same with `confine: false`:
   
   https://user-images.githubusercontent.com/33317356/202183890-304cf230-82fb-4947-a793-7134b82131da.mp4
   
   So I believe one unwanted side-effect of disabling `confine` is that the tooltip position happily overflows the top of the screen. Looking at the docs, I tested the example position they gave, and cleverly using those additional objects that are passed to the callback, it should be possible to do some exotic positioning that is as convenient as possible:
   
   ```javascript
   {
     ...,
     position: function (pos, params, dom, rect, size) {
         // tooltip will be fixed on the right if mouse hovering on the left,
         // and on the left if hovering on the right.
         var obj = {top: 60};
         obj[['left', 'right'][+(pos[0] < size.viewSize[0] / 2)]] = 5;
         return obj;
     },
   }
   ```
   https://user-images.githubusercontent.com/33317356/202188068-04a8ce49-279c-43a9-9efb-0479ed16ea4e.mp4
   
   https://echarts.apache.org/en/option.html#tooltip.position
   



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