-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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): Transition to Explore with React Router #20606
feat(dashboard): Transition to Explore with React Router #20606
Conversation
Flagging #20381 as this is related. I agree we should use a regular link for transition. The form data retrieval (and subsequent redirects) should happen on the Explore page, not dashboard page. |
I think #20381 is related to permissions, this PR is part of the Explore SPA project.
Form data retrieval does happen on Explore. What we do on dashboard is post form data with native filters, color scheme and other data related to dashboard and retrieve a form_data_key. We need to figure out a way to skip that step in order to be able to use a link. Since this change in flow (open Explore in the same tab by default) might be disruptive, I put it behind a new feature flag |
f1d9b50
to
d60f0bd
Compare
Codecov Report
@@ Coverage Diff @@
## master #20606 +/- ##
==========================================
- Coverage 66.82% 66.82% -0.01%
==========================================
Files 1752 1753 +1
Lines 65616 65633 +17
Branches 6938 6944 +6
==========================================
+ Hits 43849 43860 +11
- Misses 20007 20012 +5
- Partials 1760 1761 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
The difference in loading time is better than we expected! Some first-pass comments: I think When opening in the same tab, we can pass the |
Makes sense!
I intend to make that change while implementing |
export const getSliceHeaderTooltip = (sliceName: string | undefined) => { | ||
if (!isFeatureEnabled(FeatureFlag.DASHBOARD_TO_EXPLORE_SPA)) { | ||
return sliceName | ||
? t('Click to edit %s in ', sliceName) |
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.
? t('Click to edit %s in ', sliceName)
Is this missing a word? Edit {sliceName} in...?
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.
...in a new tab. Thanks for spotting!
I misspoke. What I meant is the form data key should be created on the Explore page (and in the Explore page's route if using SPA navigation). Context in the dashboard page should be carried over to Explore via URL params by default and only very large payload will use localStorage if needed (although I doubt it will happen very often). It's not just permissions, but how making a link depends on a post request has also made it less accessible. The post request adds a sometimes significant delay between users clicking the link to the page opening, which often confuses the users. |
Thanks for clarifying. Getting rid of that POST request is the next step for us 👍 |
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. I tested with the feature flag enabled and disabled.
Non-blocking suggestion: also add the tooltip to the Edit Chart option in the 3 dots menu.
Can you also add something to |
* feat(dashboard): Use react-router for transition to Explore + cmd click to open in new tab * Update tooltip * Add a feature flag * Update edit chart onclick event * Fix lint * Fix tests * Change feature flag name * Add tooltip to Edit chart
SUMMARY
Currently, when user clicks on chart title or "Edit chart" button in Dashboard view, we first make a POST request to get form_data_key and then open Explore in a new window. In order to fully benefit from Explore SPA project, we need to stay in the SPA context when transitioning from Dashboard to Explore - which means that we should open Explore in the same tab.
This PR changes the default behaviour to open Explore in the same tab using the React Router. If user wants to open Explore in separate tab, they can do cmd+click (on Mac) or ctrl+click (on Linux/Windows). The tooltip was adjusted to make the feature easier to discover.
Why can't we just make the chart title a link and let user "right click -> open in a new tab"? When user clicks "Edit chart", we first need to send an API request to get the form_data_key, which is required to correctly pass dashboard context to Explore. For this reason we can't use standard HTML element and instead we use a button with
onClick
callback.In the future, we'll work on improving this behaviour, so that we can use a link to transition to Explore.
Since this change in flow (open Explore in the same tab by default) might be disruptive, user can bring back the old behaviour by enabling the new feature flag
DASHBOARD_EDIT_CHART_IN_NEW_TAB
.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2022-07-05.at.12.08.44.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION