-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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(dashboard): Chart title click redirects to Explore in new tab #20111
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20111 +/- ##
=======================================
Coverage 66.46% 66.46%
=======================================
Files 1721 1721
Lines 64463 64471 +8
Branches 6794 6799 +5
=======================================
+ Hits 42844 42850 +6
- Misses 19891 19892 +1
- Partials 1728 1729 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@@ -104,9 +104,18 @@ const SliceHeader: FC<SliceHeaderProps> = ({ | |||
[crossFilterValue], | |||
); | |||
|
|||
const handleClickTitle = |
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.
Can we use useCallback/useMemo to keep the same function reference?
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 actually is the same reference! We don't create a new function here, only either assign the one from props or undefined. So unless onExploreChart
changes on every render (and if it does, we should fix it there), the reference will stay the same
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.
sliceName | ||
? t('Click to edit %s in a new tab', sliceName) | ||
: t('Click to edit chart in a new tab'), |
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.
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.
I think those checks are for the edge case where sliceName is an empty string (not sure if it's possible to achieve such state)
The purpose of keeping the "Edit chart" action item is to avoid confusion among current users who are used to going to explore via the chart menu |
The hover link isn't very discoverable so I don't think we can remove the link in 3 dots menu. I'm also curious, why do we decide to open the link in new tab when clicking from title but changed the link in the menu to open in the same tab? This kind of back and forth and inconsistency would be confusing to users, too. |
Yeah, it should be consistent... I think we should stay on the same tab in both cases |
SUMMARY
This PR makes the chart title in dashboard clickable - redirects to Explore on click. It's basically a shortcut for clicking options -> Edit chart.
On hover the title gets underlined + tooltip appears with prompt to click "Click to edit chart name in a new tab".
This effect is active only in non edit mode and when user has access to Explore
BEFORE/AFTER SCREENSHOTS
Screen.Recording.2022-05-18.at.15.09.00.mov
TESTING INSTRUCTIONS
Flow 1
Flow 2
Flow 3
ADDITIONAL INFORMATION