feat(explore): Dataset panel option tooltips #19259

kgabryje · 2022-03-18T17:57:45Z

SUMMARY

This PR introduces changes to the tooltips that appear when user hovers over metric/column label in dataset panel in Explore.

Adds description into the tooltip that appears when user hovers over metric/column name
Deletes icon that triggers tooltip with description (since description is now in the main tooltip)
Changes formatting of the text in tooltip (see screenshots)
Moves the tooltip a bit closer to the label

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TESTING INSTRUCTIONS

Add some labels and descriptions to metrics and columns, verify that tooltips display correctly

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

CC @kasiazjc

codecov · 2022-03-18T18:34:55Z

Codecov Report

Merging #19259 (24d2afa) into master (f37fc1a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #19259   +/-   ##
=======================================
  Coverage   66.65%   66.65%           
=======================================
  Files        1675     1675           
  Lines       64653    64658    +5     
  Branches     6503     6505    +2     
=======================================
+ Hits        43092    43096    +4     
  Misses      19875    19875           
- Partials     1686     1687    +1

Flag	Coverage Δ
javascript	`51.31% <100.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...erset-ui-chart-controls/src/components/Tooltip.tsx	`80.00% <ø> (ø)`
...-ui-chart-controls/src/components/ColumnOption.tsx	`84.61% <100.00%> (-1.10%)`	⬇️
...-ui-chart-controls/src/components/MetricOption.tsx	`94.44% <100.00%> (-0.30%)`	⬇️
...et-ui-chart-controls/src/components/labelUtils.tsx	`100.00% <100.00%> (+10.52%)`	⬆️
...eFilters/FiltersConfigModal/FiltersConfigModal.tsx	`63.04% <0.00%> (-1.64%)`	⬇️

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 f37fc1a...24d2afa. Read the comment docs.

rusackas · 2022-03-18T19:37:42Z

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

@@ -46,9 +46,15 @@ export const Tooltip = ({ overlayStyle, color, ...props }: TooltipProps) => {
        overlayStyle={{
          fontSize: theme.typography.sizes.s,
          lineHeight: '1.6',
+          maxWidth: 250,


If this width is relative to the tooltip's container, maybe we can do something like calc(100% - gridUnit * 4) or something?

It's not relative to tooltip's container, it's the tooltip's container 🙂 But you're right, let's use grid units

rusackas · 2022-03-18T19:38:19Z

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

@@ -46,9 +46,15 @@ export const Tooltip = ({ overlayStyle, color, ...props }: TooltipProps) => {
        overlayStyle={{
          fontSize: theme.typography.sizes.s,
          lineHeight: '1.6',
+          maxWidth: 250,
+          minWidth: 120,


Might as well make this gridUnit based, too.... it's a nice round number at least :D

rusackas · 2022-03-18T19:39:56Z

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

+    display: flex;
+    flex-direction: column;
+    font-size: ${theme.typography.sizes.s}px;
+    line-height: 1.2;


Not relevant to this PR, but we should probably add a range of line heights to our theme at some point. CC @michael-s-molina

zhaoyongjie

PR LGTM.

But I have a concern about the design. Description and name/verbose_name are used with different intents.

name/verbose_name is related to query and modeling for the dataset.
description may have a lot of content for the current column/metric, this field is unrelated to query and modeling.

The description information put into the tooltip may make this popup larger than before.

zhaoyongjie · 2022-03-21T06:01:13Z

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

+// @ts-ignore
+// eslint-disable-next-line import/no-extraneous-dependencies
+import { render, screen } from 'spec/helpers/testing-library';


Should we bring RTL into Superset-ui ?

I think it makes more sense to test this component with RTL rather than deeply comparing JS objects created by React. However, like Michael noticed in a comment in different PR, we shouldn't import from spec/ in superset-ui. I'll import RTL directly until we solve the problem of cyclic dependencies

zhaoyongjie · 2022-03-21T06:07:40Z

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

@@ -60,22 +96,27 @@ export const getMetricTooltipNode = (
  labelRef?: React.RefObject<any>,
 ): ReactNode => {
  // don't show tooltip if it hasn't verbose_name, label and hasn't truncated


update comment: // don't show tooltip if it hasn't verbose_name, label, description, and hasn't truncated

zhaoyongjie · 2022-03-21T06:12:09Z

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

@@ -36,21 +68,25 @@ export const getColumnTooltipNode = (
  labelRef?: React.RefObject<any>,
 ): ReactNode => {
  // don't show tooltip if it hasn't verbose_name and hasn't truncated


update comment: // don't show tooltip if it hasn't verbose_name, description and hasn't truncated

kgabryje · 2022-03-21T09:45:33Z

PR LGTM.

But I have a concern about the design. Description and name/verbose_name are used with different intents.

name/verbose_name is related to query and modeling for the dataset.

description may have a lot of content for the current column/metric, this field is unrelated to query and modeling.

The description information put into the tooltip may make this popup larger than before.

Hey @zhaoyongjie, thanks for comments.

Currently this component (drag&drop field) is overloaded with tooltips and it is definitely not a good user experience. Additionally, besides the tooltip the user gets while hovering over the name, we also show 4 additional/optional tooltips that appear while hovering over an icon. This is not a good flow, as:

icons are not that easy to understand or read as we may think,
this section is currently overcrowded and overwhelming with all of the details.

Although name and description may come from the different sources, they have the same context - these are basically the details of the column or metrics, so it makes sense to put them in one tooltip. Although it may become long it will still be more readable than it is now.
To make sure that long descriptions won't distract users from column name and label, I propose placing the tooltip at the bottom instead of the top by default - that way the first thing user notices is column name.

diegomedina248 · 2022-03-21T10:19:10Z

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

+  return (
+    <>
+      <TooltipSection label={t('Metric name')} text={metric.metric_name} />
+      {(metric.label || metric.verbose_name) && (


If both are falsy, but not empty, it could potentially render something.
Even if they're typed as strings (which would be fine), it's better to be sure and assert this short circuit checks to boolean values, either with !! or Boolean(condition)

I think empty strings in logical expressions always resolve to False and explicitly casting them to Boolean is redundant. Is there an example when that's not the case?

diegomedina248 · 2022-03-21T10:20:37Z

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

@@ -60,22 +96,27 @@ export const getMetricTooltipNode = (
  labelRef?: React.RefObject<any>,
 ): ReactNode => {
  // don't show tooltip if it hasn't verbose_name, label and hasn't truncated


is this comment adding anything? (we should add the description property in any rate)

diegomedina248 · 2022-03-21T10:20:55Z

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

@@ -36,21 +68,25 @@ export const getColumnTooltipNode = (
  labelRef?: React.RefObject<any>,
 ): ReactNode => {
  // don't show tooltip if it hasn't verbose_name and hasn't truncated


is this comment adding anything? (we should add the description property in any rate)

zhaoyongjie · 2022-03-21T10:42:54Z

PR LGTM.
But I have a concern about the design. Description and name/verbose_name are used with different intents.

name/verbose_name is related to query and modeling for the dataset.

description may have a lot of content for the current column/metric, this field is unrelated to query and modeling.

The description information put into the tooltip may make this popup larger than before.

Hey @zhaoyongjie, thanks for comments.

Currently this component (drag&drop field) is overloaded with tooltips and it is definitely not a good user experience. Additionally, besides the tooltip the user gets while hovering over the name, we also show 4 additional/optional tooltips that appear while hovering over an icon. This is not a good flow, as:
1. icons are not that easy to understand or read as we may think,

2. this section is currently overcrowded and overwhelming with all of the details.
Although name and description may come from the different sources, they have the same context - these are basically the details of the column or metrics, so it makes sense to put them in one tooltip. Although it may become long it will still be more readable than it is now. To make sure that long descriptions won't distract users from column name and label, I propose placing the tooltip at the bottom instead of the top by default - that way the first thing user notices is column name.

Thanks for the explanation! If this description is large, the tooltip will mask the other columns above it when the user pick another column.

Screen.Recording.2022-03-21.at.6.39.01.PM.mov

kgabryje · 2022-03-21T11:02:16Z

@zhaoyongjie Generally in Superset we let the user hover over the tooltip itself, but it's probably not a correct behaviour as the tooltip should disappear when user is not hovering over the trigger. In other words, by default we should hide the tooltip as soon as the cursor moves from the trigger, even if user tries hovering over the tooltip text. There are some cases in which such flow could be allowed (like for example filter cards in native filters, where some tooltip elements are interactive), but here it doesn't make much sense.
I'll make the tooltip visible only when user hovers over the trigger - that should fix the issue that you mentioned.
CC @kasiazjc

…anel

kgabryje · 2022-03-24T12:16:57Z

@zhaoyongjie As we discussed, tooltips disappear when you stop hovering on the label, so they shouldn't be annoying for the users when they browse the dataset panel
CC @kasiazjc

Screen.Recording.2022-03-24.at.13.14.13.mov

zhaoyongjie

LGTM

* feat(explore): Add description to column/metric tooltips in dataset panel * Fix tests * Address code review comments (cherry picked from commit 45c28c8)

* feat(explore): Add description to column/metric tooltips in dataset panel * Fix tests * Address code review comments

kgabryje requested review from rusackas, zhaoyongjie and geido March 18, 2022 17:57

pull-request-size bot added the size/L label Mar 18, 2022

rusackas reviewed Mar 18, 2022

View reviewed changes

zhaoyongjie reviewed Mar 21, 2022

View reviewed changes

diegomedina248 reviewed Mar 21, 2022

View reviewed changes

kgabryje added 3 commits March 24, 2022 12:00

feat(explore): Add description to column/metric tooltips in dataset p…

eaf1679

…anel

Fix tests

c6cb4a6

Address code review comments

24d2afa

kgabryje force-pushed the feat/dataset-panel-option-tooltips branch from d21a84b to 24d2afa Compare March 24, 2022 12:12

zhaoyongjie self-requested a review March 25, 2022 06:54

zhaoyongjie approved these changes Mar 25, 2022

View reviewed changes

kgabryje merged commit 45c28c8 into apache:master Mar 25, 2022

villebro added the lts-v1 label Mar 29, 2022

villebro pushed a commit that referenced this pull request Apr 3, 2022

feat(explore): Dataset panel option tooltips (#19259)

2f49719

* feat(explore): Add description to column/metric tooltips in dataset panel * Fix tests * Address code review comments (cherry picked from commit 45c28c8)

philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022

feat(explore): Dataset panel option tooltips (apache#19259)

3080c63

* feat(explore): Add description to column/metric tooltips in dataset panel * Fix tests * Address code review comments

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(explore): Dataset panel option tooltips #19259

feat(explore): Dataset panel option tooltips #19259

kgabryje commented Mar 18, 2022

codecov bot commented Mar 18, 2022 •

edited

rusackas Mar 18, 2022

kgabryje Mar 21, 2022

rusackas Mar 18, 2022

rusackas Mar 18, 2022

zhaoyongjie left a comment

zhaoyongjie Mar 21, 2022

kgabryje Mar 21, 2022

zhaoyongjie Mar 21, 2022

zhaoyongjie Mar 21, 2022

kgabryje commented Mar 21, 2022

diegomedina248 Mar 21, 2022

kgabryje Mar 21, 2022

diegomedina248 Mar 21, 2022

diegomedina248 Mar 21, 2022

zhaoyongjie commented Mar 21, 2022

kgabryje commented Mar 21, 2022

kgabryje commented Mar 24, 2022

zhaoyongjie left a comment

feat(explore): Dataset panel option tooltips #19259

feat(explore): Dataset panel option tooltips #19259

Conversation

kgabryje commented Mar 18, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Mar 18, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaoyongjie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgabryje commented Mar 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaoyongjie commented Mar 21, 2022

kgabryje commented Mar 21, 2022

kgabryje commented Mar 24, 2022

zhaoyongjie left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 18, 2022 •

edited