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: smart tooltip in datasourcepanel #18080

Merged
merged 8 commits into from
Feb 7, 2022

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Jan 18, 2022

SUMMARY

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

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

  1. First and foremost, create a virtual dataset in SQLlab and open it in explore.
SELECT 
'foo' as supersupersupersupersuper_long_column,
'foo' as short_column
  1. (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.

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

codecov bot commented Jan 18, 2022

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

Impacted file tree graph

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

geido commented Jan 18, 2022

/testenv up

@github-actions
Copy link
Contributor

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

@zhaoyongjie zhaoyongjie changed the title fix: unable to show tooltip on columns and metrics feat: smart tooltip in datasourcepanel Jan 20, 2022
@geido
Copy link
Member

geido commented Jan 21, 2022

/testenv up

@github-actions
Copy link
Contributor

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

michael-s-molina commented Jan 25, 2022

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

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

michael-s-molina commented Jan 25, 2022

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

zhaoyongjie commented Jan 26, 2022

@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

  1. short column name without verbose name: don't show tooltip
  2. long column name without verbose name: column name in tooltip
  3. (long or short) column name with short verbose name: column name in tooltip
  4. (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
Copy link
Member

rusackas commented Feb 7, 2022

  1. short column name without verbose name: don't show tooltip
  2. long column name without verbose name: column name in tooltip
  3. (long or short) column name with short verbose name: column name in tooltip
  4. (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
Copy link
Member Author

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

zhaoyongjie commented Feb 7, 2022

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

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

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

@zhaoyongjie
Copy link
Member Author

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

done, thanks for the review.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM. Nice set of tests!

@zhaoyongjie zhaoyongjie merged commit aa21a96 into apache:master Feb 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2022

Ephemeral environment shutdown and build artifacts deleted.

ofekisr pushed a commit to nielsen-oss/superset that referenced this pull request Feb 8, 2022
@mistercrunch 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
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 size/L 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Explore]show actual column name in tooltips on hover
5 participants