fix: Drill to detail blocked by tooltip #22082

michael-s-molina · 2022-11-09T15:24:23Z

SUMMARY

Implements a custom tooltip positioning function that ensures that the tooltip never overlaps with the pointer during normal use. In addition this fixes a bug that would cause the tooltip to stay on screen if the tooltip was visible when the user navigated away from the page.

This PR also cleans up some types on the ECharts plugin package (these kinda needed to be fixed as the code was becoming difficult to manage.

AFTER

Now tooltips are allowed to overflow other charts:

hover3.mp4

In addition, the sticky hover bug is also gone:

nav_after.mp4

BEFORE

Before, the pointer would overflow the tooltip if the chart was small enough and never utilized free space next to the chart:

overlap_before.mp4

Also navigating away from the page with the chart would sometimes leave the tooltip stuck in the middle of the screen:

nav_before.mp4

TESTING INSTRUCTIONS

Go to eph env and open the "tooltip test" dashboard.
Try to make the tooltip overlap the pointer of fall outside the screen (note that scrolling can still make the tooltip go outside)
Navigate away from the dashboard page while the tooltip is open (by using keyboard shortcuts) - the tooltip will no longer be stuck on the screen after navigation.

ADDITIONAL INFORMATION

Has associated issue

:

Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- 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

michael-s-molina · 2022-11-09T15:27:15Z

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

codecov · 2022-11-09T16:24:44Z

Codecov Report

Merging #22082 (7ad716d) into master (a77b2d6) will decrease coverage by 0.03%.
The diff coverage is 24.24%.

@@            Coverage Diff             @@
##           master   #22082      +/-   ##
==========================================
- Coverage   65.49%   65.46%   -0.04%     
==========================================
  Files        1835     1835              
  Lines       69927    69983      +56     
  Branches     7590     7610      +20     
==========================================
+ Hits        45802    45814      +12     
- Misses      22159    22203      +44     
  Partials     1966     1966

Flag	Coverage Δ
javascript	`53.75% <24.24%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...rts/src/BigNumber/BigNumberTotal/transformProps.ts	`0.00% <0.00%> (ø)`
...lugin-chart-echarts/src/BigNumber/BigNumberViz.tsx	`0.00% <0.00%> (ø)`
...lugin-chart-echarts/src/BoxPlot/EchartsBoxPlot.tsx	`0.00% <ø> (ø)`
.../plugins/plugin-chart-echarts/src/BoxPlot/types.ts	`0.00% <ø> (ø)`
.../plugin-chart-echarts/src/Funnel/EchartsFunnel.tsx	`0.00% <ø> (ø)`
...d/plugins/plugin-chart-echarts/src/Funnel/types.ts	`100.00% <ø> (ø)`
...ns/plugin-chart-echarts/src/Gauge/EchartsGauge.tsx	`0.00% <ø> (ø)`
...nd/plugins/plugin-chart-echarts/src/Gauge/types.ts	`100.00% <ø> (ø)`
...ns/plugin-chart-echarts/src/Graph/EchartsGraph.tsx	`0.00% <ø> (ø)`
...nd/plugins/plugin-chart-echarts/src/Graph/types.ts	`100.00% <ø> (ø)`
... and 28 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

villebro

Some thoughts, let me know if you wanna discuss this

superset-frontend/plugins/plugin-chart-echarts/src/defaults.ts

villebro

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 🙂

superset-frontend/plugins/plugin-chart-echarts/src/defaults.ts

michael-s-molina · 2022-11-16T13:30:53Z

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? I'll leave this open so we can collaborate.

villebro · 2022-11-23T11:53:12Z

/testenv up

github-actions · 2022-11-23T11:54:58Z

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

michael-s-molina · 2022-11-23T15:06:43Z

/testenv up

github-actions · 2022-11-23T15:10:44Z

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

michael-s-molina · 2022-11-23T15:44:53Z

superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx

 }: TimeseriesChartTransformedProps) {
  const { emitFilter, stack } = formData;
  const echartRef = useRef<EchartsHandler | null>(null);
+  // eslint-disable-next-line no-param-reassign
+  refs.echartRef = echartRef;


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

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

Echart is not using echartRef

villebro · 2022-11-23T15:48:50Z

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 = {};


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.

villebro · 2022-11-23T15:50:05Z

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


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

villebro · 2022-11-23T15:51:06Z

superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/types.ts


 export type TimeSeriesDatum = [number, number | null];
+
+export type BigNumberVizProps = {


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

villebro · 2022-11-23T15:52:06Z

superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/types.ts

-} & EchartsTitleFormData;
+} & TitleFormData;


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

villebro · 2022-11-23T15:53:30Z

superset-frontend/plugins/plugin-chart-echarts/src/Gauge/types.ts

+  BaseTransformedProps<EchartsGaugeFormData> &
+    ContextMenuTransformedProps &
+    CrossFilterTransformedProps;


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

villebro · 2022-11-23T15:55:18Z

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();


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

jinghua-qa

LGTM

kgabryje

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

github-actions · 2022-11-23T19:50:28Z

Ephemeral environment shutdown and build artifacts deleted.

fix: Drill to detail blocked by tooltip

bf5cd42

michael-s-molina requested review from kgabryje, villebro and geido November 9, 2022 15:24

pull-request-size bot added the size/XS label Nov 9, 2022

villebro reviewed Nov 11, 2022

View reviewed changes

superset-frontend/plugins/plugin-chart-echarts/src/defaults.ts Outdated Show resolved Hide resolved

villebro approved these changes Nov 16, 2022

View reviewed changes

superset-frontend/plugins/plugin-chart-echarts/src/defaults.ts Outdated Show resolved Hide resolved

villebro added 2 commits November 17, 2022 11:43

Merge branch 'master' into fix-d2d-blocked-by-tooltip

a09ec93

implement improved positioning algo

c8ce2ab

pull-request-size bot added size/M and removed size/XS labels Nov 17, 2022

villebro added 2 commits November 22, 2022 09:40

Merge branch 'master' into fix-d2d-blocked-by-tooltip

2b78498

add reference to chart div

45c7df7

pull-request-size bot added size/XL and removed size/M labels Nov 23, 2022

villebro added 2 commits November 23, 2022 10:00

Merge branch 'master' into fix-d2d-blocked-by-tooltip

060812f

add license header

f8a1e8d

pull-request-size bot added size/XXL and removed size/XL labels Nov 23, 2022

villebro added 2 commits November 23, 2022 13:00

fix tests and do more cleanup

93ce8da

more fixing

26ee293

villebro force-pushed the fix-d2d-blocked-by-tooltip branch from f331477 to 26ee293 Compare November 23, 2022 11:18

pull-request-size bot added size/XL and removed size/XXL labels Nov 23, 2022

add refs to BigNumberWithTrendline

58ccf03

add cleanup

7ad716d

michael-s-molina commented Nov 23, 2022

View reviewed changes

villebro reviewed Nov 23, 2022

View reviewed changes

michael-s-molina requested a review from jinghua-qa November 23, 2022 16:13

jinghua-qa approved these changes Nov 23, 2022

View reviewed changes

kgabryje approved these changes Nov 23, 2022

View reviewed changes

villebro merged commit 3bc0865 into apache:master Nov 23, 2022

villebro mentioned this pull request Nov 24, 2022

fix(plugin-chart-echarts): tooltip overflow bug #22218

Merged

9 tasks

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Drill to detail blocked by tooltip #22082

fix: Drill to detail blocked by tooltip #22082

michael-s-molina commented Nov 9, 2022 •

edited by villebro

Loading

michael-s-molina commented Nov 9, 2022

codecov bot commented Nov 9, 2022 •

edited

Loading

villebro left a comment

villebro left a comment

michael-s-molina commented Nov 16, 2022

villebro commented Nov 23, 2022

github-actions bot commented Nov 23, 2022

michael-s-molina commented Nov 23, 2022

github-actions bot commented Nov 23, 2022

michael-s-molina Nov 23, 2022

villebro Nov 23, 2022 •

edited

Loading

michael-s-molina Nov 23, 2022 •

edited

Loading

villebro Nov 23, 2022 •

edited

Loading

villebro Nov 23, 2022

villebro Nov 23, 2022

villebro Nov 23, 2022

villebro Nov 23, 2022

villebro Nov 23, 2022

jinghua-qa left a comment

kgabryje left a comment

github-actions bot commented Nov 23, 2022


		export type TimeSeriesDatum = [number, number \| null];

		export type BigNumberVizProps = {

fix: Drill to detail blocked by tooltip #22082

fix: Drill to detail blocked by tooltip #22082

Conversation

michael-s-molina commented Nov 9, 2022 • edited by villebro Loading

SUMMARY

AFTER

BEFORE

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

michael-s-molina commented Nov 9, 2022

codecov bot commented Nov 9, 2022 • edited Loading

Codecov Report

villebro left a comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

michael-s-molina commented Nov 16, 2022

villebro commented Nov 23, 2022

github-actions bot commented Nov 23, 2022

michael-s-molina commented Nov 23, 2022

github-actions bot commented Nov 23, 2022

michael-s-molina Nov 23, 2022

Choose a reason for hiding this comment

villebro Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

michael-s-molina Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

villebro Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

villebro Nov 23, 2022

Choose a reason for hiding this comment

villebro Nov 23, 2022

Choose a reason for hiding this comment

villebro Nov 23, 2022

Choose a reason for hiding this comment

villebro Nov 23, 2022

Choose a reason for hiding this comment

villebro Nov 23, 2022

Choose a reason for hiding this comment

jinghua-qa left a comment

Choose a reason for hiding this comment

kgabryje left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 23, 2022

michael-s-molina commented Nov 9, 2022 •

edited by villebro

Loading

codecov bot commented Nov 9, 2022 •

edited

Loading

villebro Nov 23, 2022 •

edited

Loading

michael-s-molina Nov 23, 2022 •

edited

Loading

villebro Nov 23, 2022 •

edited

Loading