Skip to content

dialog: fix button order of ConfirmSaveDialog#12559

Merged
msujew merged 4 commits intoeclipse-theia:masterfrom
vladarama:confirm-save-dialog-fixes
May 31, 2023
Merged

dialog: fix button order of ConfirmSaveDialog#12559
msujew merged 4 commits intoeclipse-theia:masterfrom
vladarama:confirm-save-dialog-fixes

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