-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
fix(dashboard): copy permalink to dashboard chart #19772
fix(dashboard): copy permalink to dashboard chart #19772
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19772 +/- ##
==========================================
+ Coverage 66.49% 66.51% +0.01%
==========================================
Files 1689 1690 +1
Lines 64614 64614
Branches 6650 6655 +5
==========================================
+ Hits 42966 42978 +12
+ Misses 19947 19936 -11
+ Partials 1701 1700 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
LGTM with a couple of nits.
Should we include this in v1.5 release? I'd assume it breaks a lot of users' workflow.
@@ -49,23 +45,17 @@ const ShareMenuItems = (props: ShareMenuItemProps) => { | |||
addDangerToast, | |||
addSuccessToast, | |||
dashboardId, | |||
formData, | |||
hash, |
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'm trying to understand what hash
means and how it is used but couldn't wrap my head around it very easily. Can it be named to something more unique so it's more searchable?
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 would have personally gone with anchor
, but
- the previous implementation used the term
hash
- I found references to
hash
elsewhere, too: https://www.w3schools.com/jsref/prop_anchor_hash.asp
So I went with hash
. Since it's already in the schema, I think it might be a good idea to leave it like that, but I'm happy to change it if you feel even remotely strongly about this.
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 it's okay to keep it as long as it is pushed downward to when the URL is actually used. Or we can add some comments.
@@ -310,13 +311,14 @@ class SliceHeaderControls extends React.PureComponent< | |||
|
|||
{supersetCanShare && ( | |||
<ShareMenuItems | |||
dashboardId={dashboardId} | |||
hash={componentId} |
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.
hash={componentId} | |
dashboardComponentId={componentId} |
Can we push down the concept of hash
to only when we interact with the URL? It took me a while to realize what it really means.
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.
That's a good idea, I like it 👍
const nativeFiltersKey = getUrlParam(URL_PARAMS.nativeFiltersKey); | ||
let filterState = {}; | ||
if (nativeFiltersKey && dashboardId) { | ||
filterState = await getFilterValue(dashboardId, nativeFiltersKey); | ||
} | ||
return getDashboardPermalink(String(dashboardId), filterState); | ||
return getDashboardPermalink(String(dashboardId), filterState, hash); |
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.
Would be nice if we make this more explicit:
return getDashboardPermalink(String(dashboardId), filterState, hash); | |
return getDashboardPermalink({ | |
dashboardId, # need to relax the type restriction in `getDashboardPermalink` as well | |
filterState, | |
hash: dashboardComponentId | |
}); |
@ktmud I believe this should address your comments: c4862e2
There's already a -1 vote on 1.5.0rc3. So if you feel this is important for 1.5.0, for the sake of formality I'd love it if you could vote a -1 in reference to this regression, so we can kill it and get rc4 up for vote asap. |
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.
LGTM!
* fix(dashboard): copy permalink to dashboard chart * lint * address comments (cherry picked from commit e061955)
* fix(dashboard): copy permalink to dashboard chart * lint * address comments
SUMMARY
The PR #18181 changed dashboard chart permalinks to link to explore view, while previously they would create a permalink to the dashboard with an anchor pointing to the chart. This changes this behavior back to how it was originally implemented.
AFTER
Now the permalink will link to the dashboard with the chart anchor:
new-chart-perma.mp4
BEFORE
Before the permalink sent the user to explore:
old-dash-perma.mp4
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION