You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/04/19 08:54:47 UTC
[GitHub] [superset] villebro opened a new pull request, #19772: fix(dashboard): copy permalink to dashboard chart
villebro opened a new pull request, #19772:
URL: https://github.com/apache/superset/pull/19772
### 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:
https://user-images.githubusercontent.com/33317356/163965896-dfa1f844-901b-4927-bebf-41120c78c076.mp4
### BEFORE
Before the permalink sent the user to explore:
https://user-images.githubusercontent.com/33317356/163966169-6f89f746-8af0-4370-848e-82121b259e4b.mp4
### TESTING INSTRUCTIONS
1. go to the Video Games Dashboard
2. Go to the "Explore Trends" tab
3. Copy a permalink from one of the charts
4. follow the permalink and see where you end up
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro merged pull request #19772: fix(dashboard): copy permalink to dashboard chart
Posted by GitBox <gi...@apache.org>.
villebro merged PR #19772:
URL: https://github.com/apache/superset/pull/19772
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a diff in pull request #19772: fix(dashboard): copy permalink to dashboard chart
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #19772:
URL: https://github.com/apache/superset/pull/19772#discussion_r853348430
##########
superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx:
##########
@@ -49,23 +45,17 @@ const ShareMenuItems = (props: ShareMenuItemProps) => {
addDangerToast,
addSuccessToast,
dashboardId,
- formData,
+ hash,
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #19772: fix(dashboard): copy permalink to dashboard chart
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #19772:
URL: https://github.com/apache/superset/pull/19772#issuecomment-1102945594
@ktmud I believe this should address your comments: https://github.com/apache/superset/pull/19772/commits/c4862e2c028bb6e32032236d017329a18fbb979e
> Should we include this in v1.5 release? I'd assume it breaks a lot of users' workflow.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #19772: fix(dashboard): copy permalink to dashboard chart
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #19772:
URL: https://github.com/apache/superset/pull/19772#discussion_r853345903
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -310,13 +311,14 @@ class SliceHeaderControls extends React.PureComponent<
{supersetCanShare && (
<ShareMenuItems
+ dashboardId={dashboardId}
+ hash={componentId}
Review Comment:
That's a good idea, I like it 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #19772: fix(dashboard): copy permalink to dashboard chart
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #19772:
URL: https://github.com/apache/superset/pull/19772#discussion_r853345097
##########
superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx:
##########
@@ -49,23 +45,17 @@ const ShareMenuItems = (props: ShareMenuItemProps) => {
addDangerToast,
addSuccessToast,
dashboardId,
- formData,
+ hash,
Review Comment:
I would have personally gone with `anchor`, but
1. the previous implementation used the term `hash`
2. 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #19772: fix(dashboard): copy permalink to dashboard chart
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #19772:
URL: https://github.com/apache/superset/pull/19772#issuecomment-1102510530
# [Codecov](https://codecov.io/gh/apache/superset/pull/19772?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19772](https://codecov.io/gh/apache/superset/pull/19772?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (783d590) into [master](https://codecov.io/gh/apache/superset/commit/594523e895a8fa455ba6db5d6cc4df80d20179a1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (594523e) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #19772 +/- ##
=======================================
Coverage 66.49% 66.49%
=======================================
Files 1689 1689
Lines 64614 64612 -2
Branches 6650 6649 -1
=======================================
Hits 42966 42966
+ Misses 19947 19946 -1
+ Partials 1701 1700 -1
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.16% <100.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19772?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...dashboard/components/SliceHeaderControls/index.tsx](https://codecov.io/gh/apache/superset/pull/19772/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1NsaWNlSGVhZGVyQ29udHJvbHMvaW5kZXgudHN4) | `66.66% <ø> (ø)` | |
| [...dashboard/components/menu/ShareMenuItems/index.tsx](https://codecov.io/gh/apache/superset/pull/19772/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL21lbnUvU2hhcmVNZW51SXRlbXMvaW5kZXgudHN4) | `90.00% <100.00%> (+8.18%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19772?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19772?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [594523e...783d590](https://codecov.io/gh/apache/superset/pull/19772?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a diff in pull request #19772: fix(dashboard): copy permalink to dashboard chart
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #19772:
URL: https://github.com/apache/superset/pull/19772#discussion_r853308220
##########
superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx:
##########
@@ -49,23 +45,17 @@ const ShareMenuItems = (props: ShareMenuItemProps) => {
addDangerToast,
addSuccessToast,
dashboardId,
- formData,
+ hash,
Review 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?
##########
superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx:
##########
@@ -49,23 +45,17 @@ const ShareMenuItems = (props: ShareMenuItemProps) => {
addDangerToast,
addSuccessToast,
dashboardId,
- formData,
+ hash,
...rest
} = props;
async function generateUrl() {
- // chart
- if (formData) {
- // we need to remove reserved dashboard url params
- return getChartPermalink(formData, RESERVED_DASHBOARD_URL_PARAMS);
- }
- // dashboard
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);
Review Comment:
Would be nice if we make this more explicit:
```suggestion
return getDashboardPermalink({
dashboardId, # need to relax the type restriction in `getDashboardPermalink` as well
filterState,
hash: dashboardComponentId
});
```
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -310,13 +311,14 @@ class SliceHeaderControls extends React.PureComponent<
{supersetCanShare && (
<ShareMenuItems
+ dashboardId={dashboardId}
+ hash={componentId}
Review Comment:
```suggestion
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org