Skip to content
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

[Dashboard] Adds Save as button to top menu #90320

Merged
merged 9 commits into from
Feb 11, 2021

Conversation

cqliu1
Copy link
Contributor

@cqliu1 cqliu1 commented Feb 4, 2021

Summary

Related to #85666.

Screen Shot 2021-02-04 at 12 36 40 PM

This adds a Save as button that triggers the dashboard save modal for editing the dashboard title, description, and tags and changes the Save button to quick save without triggering the save modal.

Changes:

  • Relabels Save button as Save as
  • Adds new Save button that performs quick save action
  • Removed Library and Create new buttons from the top menu

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@cqliu1 cqliu1 added Feature:Dashboard Dashboard related features impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.12.0 v8.0.0 release_note:enhancement labels Feb 4, 2021
@cqliu1 cqliu1 force-pushed the dashboard/save-as-button branch 4 times, most recently from 3a179e1 to 1ef43e2 Compare February 8, 2021 19:21
@cqliu1
Copy link
Contributor Author

cqliu1 commented Feb 9, 2021

@elasticmachine merge upstream

@cqliu1 cqliu1 marked this pull request as ready for review February 9, 2021 22:44
@cqliu1 cqliu1 requested a review from a team as a code owner February 9, 2021 22:44
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@ryankeairns
Copy link
Contributor

Looking good and feeling more natural, nice work.

Is there a technical reason for having both Cancel and Discard? I feel like we could just have Cancel to cover both situations:

  • if there are no pending changes, then it just switches out of Edit mode
  • if there are pending changes, then it shows the dialog to confirm continue/discard

@ryankeairns ryankeairns self-requested a review February 10, 2021 16:49
@ThomThomson
Copy link
Contributor

ThomThomson commented Feb 10, 2021

The reason there is both a cancel and discard is because during the dashboard unsaved changes PR, I noticed that the user can get into a situation where they are in view mode with unsaved changes simply by pressing the browser back button after doing their edits and I didn't want that action to discard their changes. I also wanted that to feel intentional, instead of like a bug.

The badge from #88901 will partially mitigate this by showing their changes are still around, but I thought that separating out 'cancel' and 'discard' would make it more explicit.

@cqliu1 has mentioned before that having two buttons feels redundant and I agreed, but didn't have a better solution at the time. Now I'm wondering if we can cover this by adding an extra option in the cancel dialog... something that could look like this:

Screen Shot 2021-02-10 at 2 50 55 PM

@cqliu1 & @ryankeairns, do you think this could work?

@ryankeairns
Copy link
Contributor

Ah right, thanks for the refresher @ThomThomson !
That suggestions looks good to me, but I'd like to hear from @cqliu1 on the feasibility.

@cqliu1
Copy link
Contributor Author

cqliu1 commented Feb 10, 2021

I think this should be doable. I'd prefer to handle this in a separate PR and go ahead and merge this one if we're good with the save button changes.

Just to clarify, the Keep changes button would keep the changes unsaved and switch to view mode, the Discard changes button should discard the changes and switch to view mode, and the Cancel button should just dismiss the dialog and stay in edit mode?

@ThomThomson
Copy link
Contributor

@cqliu1, that is exactly the way I envisioned it! And a followup will definitely work. I wonder if there is a better way to word keep changes that makes it obvious we are still leaving Edit mode.

@ryankeairns
Copy link
Contributor

ryankeairns commented Feb 10, 2021

We can hash it out further in the next PR, but one idea comes to min:

  • Keep unsaved changes which is how we refer to them on the Dashboard home page callout, or
  • Switch to view mode which is in effect what you're doing. For the latter, an iconTooltip might be helpful as well to indicate that changes will remain in Edit mode.

@kmartastic
Copy link
Contributor

  • Switch to view mode which is in effect what you're doing. For the latter, an iconTooltip might be helpful as well to indicate that changes will remain in Edit mode.

My 2 cents. I am not a fan of "unsaved changes" persisting while leaving Edit mode and going back to View mode.

As a customer, I go to edit mode to make changes, I decide not to, and hit cancel. I go back to View mode.
As a customer, I go to edit mode to make changes, I make a change, but I don't like it, I hit cancel. I go back to View mode.
As a customer, I go to edit mode to make changes, I make a change, I like the change, I hit save and return. I go back to View mode.
As a customer, I go to edit mode to make changes, I make a change, I like the change, I hit save. I want to keep making changes, I stay in Edit mode.

It starts to become fringe for us to support unsaved changes, while allowing a customer to navigate back to View mode. The trade-off here is flexibility for the customer vs complexity for the customer to be aware of (i.e. they go back to Edit mode and discover previous edits, that would be surprising to many I would guess).

Happy to chat about this further, there is likely other context that I am not aware of.

@ThomThomson
Copy link
Contributor

The reason this difference exists in the first place is mostly technical. The edit mode is stored in the URL, so the user can change it simply by using the back and forward browser buttons. Fully discarding any unsaved changes shouldn't be done without a warning / confirmation, and it would introduce design challenges to show a modal when the back button is pressed, and then potentially stop the navigation.

I agree that if a user has decided to 'cancel' out of edit mode, they are more than likely looking to discard the changes. Perhaps switching the default call to action on the modal from Keep changes to Discard changes would be sufficient to allow the flexibility of seeing the last saved state without losing your changes (and keeping the implementation simpler) while avoiding the complexity of having to remember that canceling out doesn't always discard.

Screen Shot 2021-02-10 at 6 12 54 PM

As a customer, I go to edit mode to make changes, I make a change, I like the change, I hit save. I want to keep making changes, I stay in Edit mode.

This is actually a different, but equally interesting discussion. I can see this being true, but it currently does not work that way. Saving will always boot you out of edit mode, though it is trivial to get back in.

@ryankeairns
Copy link
Contributor

+1 to keeping the conversation alive post merge. There are some things we've learned to live with here that are atypical / could be improved upon.

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is looking great! Tested locally on chrome. I have a few small changes that will help this editing experience align with the others.

  1. When creating a new item in Visualize, Lens & Maps, the main CTA button is named save whereas on dashboard it's currently named save as
  2. In Maps / Vis / Lens, when editing an existing item, the 'save as' button opens with 'save as new ...` preselected.

Overall, everything works as expected, and the quicksave is an awesome addition. This will help us make 7.12.0 feel vastly different / better!

@cqliu1
Copy link
Contributor Author

cqliu1 commented Feb 11, 2021

@ThomThomson I updated the label when creating a new dashboard.

Screen Shot 2021-02-11 at 9 45 48 AM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dashboard 177.6KB 178.4KB +866.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@cqliu1 cqliu1 merged commit 30e86ac into elastic:master Feb 11, 2021
@cqliu1 cqliu1 deleted the dashboard/save-as-button branch February 11, 2021 20:17
cqliu1 added a commit to cqliu1/kibana that referenced this pull request Feb 11, 2021
cqliu1 added a commit that referenced this pull request Feb 11, 2021
@kmartastic
Copy link
Contributor

though it is trivial to get back in

I appreciate that -- and yes, move your mouse and click Edit again is trivial.
To the customer, it's unnecessary, if not annoying, and potentially... an example of a poorly designed product.

Let's continue the discussion.

@ThomThomson
Copy link
Contributor

You're absolutely right. If you look at other products, saving doesn't boot you out of edit mode, and we should try to be as consistent with the wider ecosystem as possible. I think a meeting would be ideal for us all to talk through the behaviour of all the dashboard top nav buttons, and settle on all of the improvements that need to be made, as well as a time frame for these changes.

Dashboard is meant to be the easier / more out of the box presentation tool, so it's important that we make it as intuitive as possible.

@cqliu1 cqliu1 restored the dashboard/save-as-button branch June 8, 2022 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort release_note:enhancement review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants