-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add a tooltip to plots with long titles that are cut by Vega #4840
Conversation
export const ZoomablePlotWrapper: React.FC< | ||
PropsWithChildren<ZoomablePlotWrapperProps> | ||
> = ({ title, id, children }) => { | ||
const isTitleCut = title?.indexOf('…') === 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL we aren't doing this in code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no wait.... we are doing this in code but you can bypass because the id is the title, got it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to extend this for any truncation when the big template update lands.
I.e tooltip could contain
Title: ${title}
y-axis: ${yLabel}
...
if any are truncated.
const isTitleCut = title?.indexOf('…') === 0 | ||
|
||
return isTitleCut ? ( | ||
<Tooltip content={id} placement="top" interactive> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please confirm that having a tooltip in place does not break the ability to zoom the plot 🙏🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not prevent from zooming the plot. There should also be a tooltip on the zoomed plot though. I'll add one there as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Thanks @sroy3 for addressing this so promptly. |
I can do a follow up for this. |
I feel like this would pollute the UI without adding any real value. Especially if all your plots have short titles. The tooltip appears instantly when hovering a plot with a title that's been cut off. It is a common UX pattern to hover to view missing information, I don't think people will feel lost about this. |
Code Climate has analyzed commit 35121fa and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.0% (0.0% change). View more on Code Climate. |
Demo
Screen.Recording.2023-10-16.at.11.47.41.AM.mov