Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(explore): Dataset panel option tooltips #19259

Merged
merged 3 commits into from
Mar 25, 2022

Conversation

kgabryje
Copy link
Member

SUMMARY

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

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

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
image
image

After:
image
image
image

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

codecov bot commented Mar 18, 2022

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR LGTM.

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

  1. name/verbose_name is related to query and modeling for the dataset.
  2. 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.

Comment on lines 21 to 23
// @ts-ignore
// eslint-disable-next-line import/no-extraneous-dependencies
import { render, screen } from 'spec/helpers/testing-library';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we bring RTL into Superset-ui ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kgabryje
Copy link
Member Author

PR LGTM.

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

  1. name/verbose_name is related to query and modeling for the dataset.
  2. 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.
image

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@zhaoyongjie
Copy link
Member

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

  1. name/verbose_name is related to query and modeling for the dataset.
  2. 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. image

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
Copy link
Member Author

@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

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

@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 zhaoyongjie self-requested a review March 25, 2022 06:54
Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kgabryje kgabryje merged commit 45c28c8 into apache:master Mar 25, 2022
villebro pushed a commit that referenced this pull request Apr 3, 2022
* 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): Add description to column/metric tooltips in dataset panel

* Fix tests

* Address code review comments
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels lts-v1 size/L 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants