Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat: add tooltip for full metric names #1066

Merged
merged 1 commit into from
Apr 20, 2021
Merged

Conversation

etr2460
Copy link
Contributor

@etr2460 etr2460 commented Apr 20, 2021

🏆 Enhancements

Adds a tooltip to the metric name so that if it overflows the container we can still read it.

Test plan:
CI. new unit test, test in superset
Screen Shot 2021-04-20 at 10 54 11 AM
Screen Shot 2021-04-20 at 10 55 03 AM

to: @pkdotson @ktmud @graceguo-supercat

@etr2460 etr2460 requested a review from a team as a code owner April 20, 2021 17:56
@vercel
Copy link

vercel bot commented Apr 20, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/9mySimPrvCGCVg4gYfmBdPXECGEe
✅ Preview: https://superset-ui-git-erik-ritter-metric-tooltip-superset.vercel.app

@codecov
Copy link

codecov bot commented Apr 20, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@6226d2f). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1066   +/-   ##
=========================================
  Coverage          ?   27.80%           
=========================================
  Files             ?      453           
  Lines             ?     9104           
  Branches          ?     1416           
=========================================
  Hits              ?     2531           
  Misses            ?     6382           
  Partials          ?      191           
Impacted Files Coverage Δ
...-ui-chart-controls/src/components/MetricOption.tsx 56.25% <ø> (ø)

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 6226d2f...ccff1fb. Read the comment docs.

Copy link
Contributor

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

Looks like a reasonable change. We already have tooltips in the Dataset panel.

Although I'm personally a little more in favor of using html title attribute over tooltips for text cutoffs, especially when most of the time the text can be fully displayed. It'd be a little distracting seeing too many tooltips popping out when you move your mouse.

@etr2460
Copy link
Contributor Author

etr2460 commented Apr 20, 2021

@ktmud in general i'd agree with using the title attribute, but i've always found it so infuriating that it takes ~1 second for the tooltip to actually display on hover. Since we have tooltips everywhere else already (including on ad hoc metric names, this is only for saved metrics) i'm going to stick with this for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants