fix(Dashboard): Support "Edit chart" click on a new window #28054
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/testenv up |
|
@geido Ephemeral environment spinning up at http://52.37.25.121:8080. Credentials are |
eschutho
approved these changes
Apr 16, 2024
rusackas
reviewed
Apr 16, 2024
| @@ -590,7 +591,12 @@ const SliceHeaderControls = (props: SliceHeaderControlsPropsWithRouter) => { | |||
| case MenuKeys.ExploreChart: | |||
| // eslint-disable-next-line no-unused-expressions | |||
| props.logExploreChart?.(props.slice.slice_id); | |||
| window.open(props.exploreUrl); | |||
| if (domEvent.metaKey || domEvent.ctrlKey) { | |||
There was a problem hiding this comment.
A few questions here:
- Are we sure this works for mac and PC browsers as expected? Seems like it should, but ¯\_(ツ)_/¯
- Do we need tests to prevent this regressing again?
- Is there a reason we can't just use an HTML link here? I feel like we had that solved at one point.
- Is it not amazing that this particular link has been the subject of like 20 PRs and Issues?
There was a problem hiding this comment.
- Yes it should
- Let me see what kind of tests I can add here
- We now enable dropdown navigation for accessibility by controlling onClick handlers
- Tests should hopefully fix this
There was a problem hiding this comment.
I will add the unit tests in a follow-up PR to unblock this bug fix
|
Ephemeral environment shutdown and build artifacts deleted. |
betodealmeida
pushed a commit
that referenced
this pull request
Apr 25, 2024
(cherry picked from commit 89da4f8)
qleroy
pushed a commit
to qleroy/superset
that referenced
this pull request
Apr 28, 2024
jzhao62
pushed a commit
to jzhao62/superset
that referenced
this pull request
May 16, 2024
eschutho
pushed a commit
that referenced
this pull request
Jun 5, 2024
(cherry picked from commit 89da4f8)
vinothkumar66
pushed a commit
to vinothkumar66/superset
that referenced
this pull request
Nov 11, 2024
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
This PR #26138 removed the link from the "Edit chart" menu item on dashboards to enhance accessibility but it caused the chart to open in a new window. The goal of this PR is to support opening the chart in the same window by default while still allowing for opening a chart in a separate window with
cmd + click.It also removes an
onBlurhandler that was causing the interruption of some menu item actions by closing the dropdown abruptly.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N.A.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION