feat: smart tooltip in datasourcepanel #18080

zhaoyongjie · 2022-01-18T13:13:42Z

SUMMARY

This PR intends to resolve 2 potential issues in the current Superset

there is no chance to show column name and metric name in the datapanel if defining a verbose name for these.

can.t.show.column.name.mov

superset-frontend/src/explore/components/DatasourcePanel/index.tsx::LabelContainer will re-render when we trigger from onMouseLeave or onMouseEnter event.

This simple experiment can prove it.

(superset) yongjie.zhao@:superset-frontend$ git diff
diff --git a/superset-frontend/src/explore/components/DatasourcePanel/index.tsx b/superset-frontend/src/explore/components/DatasourcePanel/index.tsx
index 0a3cf7b76..57b9627a1 100644
--- a/superset-frontend/src/explore/components/DatasourcePanel/index.tsx
+++ b/superset-frontend/src/explore/components/DatasourcePanel/index.tsx
@@ -122,6 +122,7 @@ const LabelContainer = (props: {
   children: React.ReactElement;
   className: string;
 }) => {
+  console.log("Hey, i'm Label Container");
   const labelRef = useRef<HTMLDivElement>(null);
   const [showTooltip, setShowTooltip] = useState(true);
   const isLabelTruncated = () =>

rerender.in.datapanel.mov

closes: #13252

This PR removed some redundant logic to fix this.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After

default tooltip

default.tooltip.mov

add verbose name

show.verbose.and.original.column.mov

TESTING INSTRUCTIONS

First and foremost, create a virtual dataset in SQLlab and open it in explore.

SELECT 
'foo' as supersupersupersupersuper_long_column,
'foo' as short_column

(default) verify that
a) There is a tooltip on the supersupersupersupersuper_long_column
b) There isn't a tooltip on the short_column
c) There isn't a tooltip on the count
d) there aren't tooltips when you scale datasouce-panel up.
verify verbose name name
a) tooltip will always appear column(metric) name and verbose name if you set verbose name in columns or metrics(except verbose truncated)

ADDITIONAL INFORMATION

Has associated issue: closes: [Explore]show actual column name in tooltips on hover #13252
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

codecov · 2022-01-18T14:48:35Z

Codecov Report

Merging #18080 (4f3d9eb) into master (151d30b) will increase coverage by 0.06%.
The diff coverage is 87.09%.

❗ Current head 4f3d9eb differs from pull request most recent head e4e1eca. Consider uploading reports for the commit e4e1eca to get more accurate results

@@            Coverage Diff             @@
##           master   #18080      +/-   ##
==========================================
+ Coverage   66.23%   66.30%   +0.06%     
==========================================
  Files        1594     1595       +1     
  Lines       62620    62630      +10     
  Branches     6310     6308       -2     
==========================================
+ Hits        41479    41525      +46     
+ Misses      19493    19454      -39     
- Partials     1648     1651       +3

Flag	Coverage Δ
javascript	`51.36% <87.09%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
.../controls/DndColumnSelectControl/OptionWrapper.tsx	`63.41% <ø> (ø)`
superset/charts/api.py	`85.93% <ø> (ø)`
superset/dashboards/api.py	`92.54% <ø> (ø)`
superset/databases/api.py	`93.99% <ø> (ø)`
superset/datasets/api.py	`91.89% <ø> (ø)`
superset/queries/saved_queries/api.py	`95.28% <ø> (ø)`
.../explore/components/ExploreViewContainer/index.jsx	`57.14% <33.33%> (-0.08%)`	⬇️
...et-ui-chart-controls/src/components/labelUtils.tsx	`89.47% <89.47%> (ø)`
...-ui-chart-controls/src/components/ColumnOption.tsx	`85.71% <100.00%> (-1.79%)`	⬇️
...-ui-chart-controls/src/components/MetricOption.tsx	`94.73% <100.00%> (+0.29%)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 151d30b...e4e1eca. Read the comment docs.

geido · 2022-01-18T16:01:12Z

/testenv up

github-actions · 2022-01-18T16:03:56Z

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

superset-frontend/src/explore/components/DatasourcePanel/index.tsx

superset-frontend/packages/superset-ui-chart-controls/src/components/labelUtils.ts

geido · 2022-01-21T14:18:03Z

/testenv up

github-actions · 2022-01-21T14:37:30Z

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

michael-s-molina · 2022-01-25T14:11:50Z

@zhaoyongjie One thing that I found weird was that when the text is not truncated and I hover a column, for example, I get the original column name but when the text is truncated I only get the verbose name. I was expecting to see both the original column name and the verbose name. This applies to both columns and metrics.

zhaoyongjie · 2022-01-25T17:24:00Z

@zhaoyongjie One thing that I found weird was that when the text is not truncated and I hover a column, for example, I get the original column name but when the text is truncated I only get the verbose name. I was expecting to see both the original column name and the verbose name. This applies to both columns and metrics.

Hi @michael-s-molina, If a column has a verbose name and it is un-truncated, it will show column name. if a column has a verbose name and it truncated, it will show un-truncated verbose name

Currently, tooltip just show verbose name or database column name, not both.

michael-s-molina · 2022-01-25T17:29:35Z

@zhaoyongjie One thing that I found weird was that when the text is not truncated and I hover a column, for example, I get the original column name but when the text is truncated I only get the verbose name. I was expecting to see both the original column name and the verbose name. This applies to both columns and metrics.

Hi @michael-s-molina, If a column has a verbose name and it is un-truncated, it will show column name. if a column has a verbose name and it truncated, it will show un-truncated verbose name

Currently, tooltip just show verbose name or database column name, not both.

I think they are different information and both should be present. One thing is to see the original column name which is important and should always be visible. Another thing is to see the full truncated text which should display if the text gets truncated.

Here's one example:

I have a column with the name Job Location Preference According to the National Job Department
The original column name is job_location_preference

As the name will get truncated, the tooltip would display:

verbose name: Job Location Preference According to the National Job Department
column name: job_location_preference

zhaoyongjie · 2022-01-26T02:34:06Z

@zhaoyongjie One thing that I found weird was that when the text is not truncated and I hover a column, for example, I get the original column name but when the text is truncated I only get the verbose name. I was expecting to see both the original column name and the verbose name. This applies to both columns and metrics.

Hi @michael-s-molina, If a column has a verbose name and it is un-truncated, it will show column name. if a column has a verbose name and it truncated, it will show un-truncated verbose name
Currently, tooltip just show verbose name or database column name, not both.

I think they are different information and both should be present. One thing is to see the original column name which is important and should always be visible. Another thing is to see the full truncated text which should display if the text gets truncated.

Here's one example:
* I have a column with the name `Job Location Preference According to the National Job Department `

* The original column name is `job_location_preference`
As the name will get truncated, the tooltip would display:
* verbose name: Job Location Preference According to the National Job Department

* column name: job_location_preference

I know these different identifiers. Currently, only 1 name shown in the tooltip. let me list all the possibilities

short column name without verbose name: don't show tooltip
long column name without verbose name: column name in tooltip
(long or short) column name with short verbose name: column name in tooltip
(long or short) column name with long verbose name: verbose name in tooltip

Notice that long names will be truncated.

The major discussion is 4. How to display tooltip when a column with a truncated verbose name. What do you think @geido @rusackas

rusackas · 2022-02-07T06:13:17Z

short column name without verbose name: don't show tooltip

long column name without verbose name: column name in tooltip

(long or short) column name with short verbose name: column name in tooltip

(long or short) column name with long verbose name: verbose name in tooltip

If I'm not mistaken (4) means you no longer have access to see the column name. I think we should indeed show the (non-truncated) verbose name AND the column name in the tooltip. This is weird, because it's not consistent. But, I don't think it's a good idea to hide information users might see as important.

Another option I think I would prefer, is to make 3&4 consistent. This would mean showing the Verbose AND Column names in (3) and (4). For 3, the information is a bit redundant, but it at least establishes a regular pattern, so users don't have to think as much about what they're seeing as they go through the list.

zhaoyongjie · 2022-02-07T06:27:55Z

If I'm not mistaken (4) means you no longer have access to see the column name. I think we should indeed show the (non-truncated) verbose name AND the column name in the tooltip. This is weird, because it's not consistent. But, I don't think it's a good idea to hide information users might see as important.

Another option I think I would prefer, is to make 3&4 consistent. This would mean showing the Verbose AND Column names in (3) and (4). For 3, the information is a bit redundant, but it at least establishes a regular pattern, so users don't have to think as much about what they're seeing as they go through the list.

Nice method, I will update this PR for cases (3) and (4). Always showing verbose name and column name in the tooltip if column/metric has a verbose name.

zhaoyongjie · 2022-02-07T10:08:24Z

@michael-s-molina @rusackas done this change. please review again. Thanks a lot.

michael-s-molina

Just need to fix the typo and maybe revisit the tests descriptions to account for the latest changes.

superset-frontend/packages/superset-ui-chart-controls/test/components/labelUtils.test.tsx

superset-frontend/packages/superset-ui-chart-controls/src/components/labelUtils.tsx

zhaoyongjie · 2022-02-07T13:34:02Z

Just need to fix the typo and maybe revisit the tests descriptions to account for the latest changes.

done, thanks for the review.

michael-s-molina

LGTM. Nice set of tests!

github-actions · 2022-02-07T14:48:52Z

Ephemeral environment shutdown and build artifacts deleted.

pull-request-size bot added the size/M label Jan 18, 2022

zhaoyongjie requested a review from geido January 18, 2022 15:25

geido reviewed Jan 18, 2022

View reviewed changes

superset-frontend/src/explore/components/DatasourcePanel/index.tsx Show resolved Hide resolved

pull-request-size bot added size/L and removed size/M labels Jan 19, 2022

zhaoyongjie requested a review from geido January 19, 2022 15:38

geido reviewed Jan 19, 2022

View reviewed changes

superset-frontend/packages/superset-ui-chart-controls/src/components/labelUtils.ts Outdated Show resolved Hide resolved

zhaoyongjie requested review from geido and kgabryje January 20, 2022 14:07

zhaoyongjie changed the title ~~fix: unable to show tooltip on columns and metrics~~ feat: smart tooltip in datasourcepanel Jan 20, 2022

zhaoyongjie added 6 commits February 7, 2022 15:22

fix: unable to show tooltip on columns and metrics

e9e5468

wip

412fe3c

fix truncated metric or column

05a6b26

add test

c8597e3

update panel if resize

5568311

typo

cc2b81c

zhaoyongjie force-pushed the fix_tooltip_popover branch from 22ec75c to cc2b81c Compare February 7, 2022 07:22

show verbose name and ds name

c6dc69c

zhaoyongjie requested a review from rusackas February 7, 2022 09:43

zhaoyongjie requested a review from michael-s-molina February 7, 2022 10:05

michael-s-molina reviewed Feb 7, 2022

View reviewed changes

superset-frontend/packages/superset-ui-chart-controls/test/components/labelUtils.test.tsx Outdated Show resolved Hide resolved

superset-frontend/packages/superset-ui-chart-controls/src/components/labelUtils.tsx Outdated Show resolved Hide resolved

typos

e4e1eca

michael-s-molina approved these changes Feb 7, 2022

View reviewed changes

zhaoyongjie merged commit aa21a96 into apache:master Feb 7, 2022

ofekisr pushed a commit to nielsen-oss/superset that referenced this pull request Feb 8, 2022

feat: smart tooltip in datasourcepanel (apache#18080)

d7b753c

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: smart tooltip in datasourcepanel #18080

feat: smart tooltip in datasourcepanel #18080

zhaoyongjie commented Jan 18, 2022 •

edited

Loading

codecov bot commented Jan 18, 2022 •

edited

Loading

geido commented Jan 18, 2022

github-actions bot commented Jan 18, 2022

geido commented Jan 21, 2022

github-actions bot commented Jan 21, 2022

michael-s-molina commented Jan 25, 2022 •

edited

Loading

zhaoyongjie commented Jan 25, 2022

michael-s-molina commented Jan 25, 2022 •

edited

Loading

zhaoyongjie commented Jan 26, 2022 •

edited

Loading

rusackas commented Feb 7, 2022

zhaoyongjie commented Feb 7, 2022

zhaoyongjie commented Feb 7, 2022 •

edited by michael-s-molina

Loading

michael-s-molina left a comment

zhaoyongjie commented Feb 7, 2022

michael-s-molina left a comment

github-actions bot commented Feb 7, 2022

feat: smart tooltip in datasourcepanel #18080

feat: smart tooltip in datasourcepanel #18080

Conversation

zhaoyongjie commented Jan 18, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After

default tooltip

add verbose name

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jan 18, 2022 • edited Loading

Codecov Report

geido commented Jan 18, 2022

github-actions bot commented Jan 18, 2022

geido commented Jan 21, 2022

github-actions bot commented Jan 21, 2022

michael-s-molina commented Jan 25, 2022 • edited Loading

zhaoyongjie commented Jan 25, 2022

michael-s-molina commented Jan 25, 2022 • edited Loading

zhaoyongjie commented Jan 26, 2022 • edited Loading

rusackas commented Feb 7, 2022

zhaoyongjie commented Feb 7, 2022

zhaoyongjie commented Feb 7, 2022 • edited by michael-s-molina Loading

michael-s-molina left a comment

Choose a reason for hiding this comment

zhaoyongjie commented Feb 7, 2022

michael-s-molina left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 7, 2022

zhaoyongjie commented Jan 18, 2022 •

edited

Loading

codecov bot commented Jan 18, 2022 •

edited

Loading

michael-s-molina commented Jan 25, 2022 •

edited

Loading

michael-s-molina commented Jan 25, 2022 •

edited

Loading

zhaoyongjie commented Jan 26, 2022 •

edited

Loading

zhaoyongjie commented Feb 7, 2022 •

edited by michael-s-molina

Loading