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.
|
| @@ -49,23 +45,17 @@ const ShareMenuItems = (props: ShareMenuItemProps) => { | |||
| addDangerToast, | |||
| addSuccessToast, | |||
| dashboardId, | |||
| formData, | |||
| hash, | |||
There was a problem hiding this comment.
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.
I would have personally gone with anchor, but
- the previous implementation used the term
hash - I found references to
hashelsewhere, 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.
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.
| 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.
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.
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. |
* 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
mistercrunch
added
🍒 1.5.0
🍒 1.5.1
🍒 1.5.2
🍒 1.5.3
🏷️ bot
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