-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Time to Visualize] Remove Panels from URL #86939
[Time to Visualize] Remove Panels from URL #86939
Conversation
c12eab3
to
946c7c4
Compare
…its. Changed edit link in listing page to actually edit. Removed unsaved edits when dashboard is not dirty
59a2514
to
ea9162d
Compare
Pinging @elastic/kibana-presentation (Team:Presentation) |
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.
Tested a bit, mostly was focused on stuff around state management / URL syncing
Noticed that and assume is by design:
- browser history navigation doesn't work anymore more for panel changes
Think those are bugs:
- "cancel" in edit mode no longer works for time range changes. I checked 7.10.2 and it worked. Didn't check master, possible it regressed before this pr
- Looks like "cancel" does something weird now with state in the URL. removes
_g
part from the URL and adds multiple not relevant history records - You can get into a bad state if press "back" just after saving a new dashboard. Maybe makes sense to use "replace" there.
src/plugins/dashboard/public/application/listing/dashboard_unsaved_listing.test.tsx
Show resolved
Hide resolved
We actually already have one of these in I will keep track of the bugs you've discovered - as well as another I found.
|
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.
Looks pretty good to me! However, I did also run into the issue where after saving a new dashboard, hitting the back back button takes me to a blank dashboard
Will definitely fix all identified issues before merge! Thanks for looking into it |
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.
Approving the SASS and design aspects. As was discussed, there are some nice-to-haves (e.g. discard all) that can be tracked separately. Likewise, we can iterate on the layout once the page layout service becomes available.
Great work here! Losing Dashboards is a longstanding, frustrating experience.
@elasticmachine merge upstream |
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.
Code review and testing:
Found some things that I think worth addressing + some nits.
In this dialog:
if I click "continue editing" it is just closing the dialog. I'd assume it either should navigate to the dashboard or button text should be changed
-
Edits to dashboard filters/query/time aren't saved when I return to them from the listing. I assume this is expected behavior now, but I thought I'd call it out.
-
When you edit a dashboard, then go to the listing page, and instead of continue editing it from the draft list navigate to it and then edit or just click edit from a normal list, then you just implicitly jump to the last edit state. I think that could be confusing in some cases and a dialog that explicitly asks if the user wants to restore the latest draft or discard it would be nice to have. Probably related to [Time to Visualize] Indicate unsaved changes #88901
-
Noticed that drafts dialog gets clunky on smaller screen:
- Also imo it is still worth adding some error handling on reading/writing sessionStorage.
src/plugins/dashboard/public/application/listing/dashboard_unsaved_listing.tsx
Outdated
Show resolved
Hide resolved
src/plugins/dashboard/public/application/listing/dashboard_unsaved_listing.tsx
Outdated
Show resolved
Hide resolved
export function createDashboardEditUrl(id: string) { | ||
return `${DashboardConstants.VIEW_DASHBOARD_URL}/${id}`; | ||
export function createDashboardEditUrl(id?: string, editMode?: boolean) { | ||
const edit = editMode ? `?${DASHBOARD_STATE_STORAGE_KEY}=(viewMode:edit)` : ''; |
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.
nit: you can use setStateToKbnUrl
function from kibana_utils
to set state in proper format
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 thought about using this, but it seems to prepend the entire current URL / not be used for routing within apps. I might be using it wrong though.
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 thought it should also work like this:
setStateToKbnUrl('_a', {viewMode: 'edit'}, {storeInHashQuery: false}, '/dashboard/dashboard-id')
but I am not 100% sure now. feel free to skip .
p.s. hate this weird fn signature 😅
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.
Yeah that did work, the trick was the storeInHashQuery: false
argument. I will leave it in though it does seem to be adding quite a bit of complexity to this function.
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 actually had to revert this. Turns out importing the stateToKbnUrl
method in that file causes a build failure in x-pack because of a functional test file using this. I ran into this before as well but forgot!
src/plugins/dashboard/public/application/listing/dashboard_unsaved_listing.tsx
Show resolved
Hide resolved
src/plugins/dashboard/public/application/listing/dashboard_unsaved_listing.tsx
Show resolved
Hide resolved
@ThomThomson here is a PR with some responsive enhancements. Feel free to merge if it looks good to you. After |
Add responsive styles
@ryankeairns, very quick on the draw & looking great. Merged this. @Dosant, I am working through the suggestions now, but have some initial responses:
|
@elasticmachine merge upstream |
…kibana into feature/removePanelsFromUrl
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.
Retested listing page since this is where most of the change since the last review were,
one comment:
useMount(() => { | ||
setDashboardIds(dashboardPanelStorage.getDashboardIdsWithUnsavedChanges()); | ||
useEffect(() => { | ||
return () => setMounted(false); |
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.
This runs on every state change. If you wanted something like componentWillUnmount
then it would have to be implemented a bit different: https://github.com/streamich/react-use/blob/master/src/useUnmount.ts
But as I understand, we need this for request cancelation, then the best would be to handle it as part of the effect with the request
useEffect(() => {
let canceled = false;
request().then(() => {
if (canceled) return;
})
return () => {
canceled = true;
}
})
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.
Wow, huh. I meant to pass in an empty dependency array there, which would have made it run only on unmount. I need to be more careful with these things. Thanks for noticing this!
That said, the approach of canceling within the useEffect is cleaner anyway. I have implemented it.
jenkins test this |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Removed panels from dashboard URLs Co-authored-by: Ryan Keairns <[email protected]> # Conflicts: # test/functional/apps/dashboard/view_edit.ts
Removed panels from dashboard URLs Co-authored-by: Ryan Keairns <[email protected]>
Summary
Fixes #71499
This PR accomplishes a number of things:
Note: The following changes will only be visible IfEdit: This option is now set to true by defaultdashboard.allowByValueEmbeddables
is set to true inconfig/kibana.dev.yml
To do: Functional Tests are incoming. Required functional tests are linked in #80584
Testing Ideas
This PR has a lot of UX, so it's important we test every single edge case we can think of. Some edge cases I can think of include:
Checklist
Delete any items that are not applicable to this PR.
For maintainers