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

dialog: fix button order of ConfirmSaveDialog #12559

Merged
merged 4 commits into from
May 31, 2023

Conversation

vladarama
Copy link
Contributor

@vladarama vladarama commented May 24, 2023

What it does

This PR fixes #12246 by making changes to the UI as per the defined conventions. It reorders the dialog buttons in the ConfirmSaveDialog to follow Cancel > Don't save > Save all sequence. This PR also makes sure that only one primary button is highlighted, which is the Save all button.

How to test

  1. Remove AutoSave: File -> AutoSave
  2. Create a new untitled file and make it dirty or make changes to an existing file
  3. Attempt to close the app or close the workspace
  4. Notice the dialog and test all three options ( Cancel > Don't save > Save all)
  5. Make sure that all three options work on both browser and electron

Review checklist

Reminder for reviewers

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I'm not really sure that this is the correct approach to this issue. First off, we have some regressions with this:

image

I believe a refactoring of the ConfirmSaveDialog is in order. I'm not perfectly sure that the dialog should be in charge of actually executing the change. Instead, I would like to see it being decoupled from ConfirmDialog and returning some sort of enum:

enum ConfirmSaveResult {
  Cancel,
  Save,
  DontSave
}

That way we can actually save outside of the dialog and also make it easier to order the buttons, without needing to touch the ConfirmDialog at all.

@vladarama
Copy link
Contributor Author

Yeah, you are right, refactoring ConfirmSaveDialog is the better approach. I'll get on it.

@vladarama vladarama requested a review from msujew May 25, 2023 20:22
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I think I led you down the wrong path. I've refactored this a bit more to reduce some of the duplication. Looks good to me now 👍

@vladarama
Copy link
Contributor Author

Thanks for the changes Mark, I realize how much I overcomplicated some stuff.

@msujew msujew merged commit 085c3e2 into eclipse-theia:master May 31, 2023
@vince-fugnitto vince-fugnitto added dialogs issues related to dialogs ui/ux issues related to user interface / user experience labels Jun 14, 2023
@vladarama vladarama deleted the confirm-save-dialog-fixes branch June 14, 2023 19:39
@vince-fugnitto vince-fugnitto added this to the 1.39.0 milestone Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialogs issues related to dialogs ui/ux issues related to user interface / user experience
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

dialog: improve user experience of ConfirmSaveDialog
3 participants