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

Save changes before closing interaction. #12166

Merged

Conversation

emilhammarstedtst
Copy link
Contributor

@emilhammarstedtst emilhammarstedtst commented Feb 9, 2023

Screenshot from 2023-02-09 13-44-36

What it does

  • Question to save all changes, cancel or discard all changes before closing
  • Display what is not yet saved.
  • Reorder options and reword the question and options to be clear of the outcome of each action
  • Implemented using ConfirmDialogSave extending ConfirmDialog

How to test

Electron build of theia and make one or more editors dirty then click the "X" to close the theia window. the popup should appear with a question to save.

Review checklist

Reminder for reviewers

Contributed by STMicroelectronics

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@emilhammarstedtst thank you for the contribution 👍

I have some general comments about the dialog, mainly to align with the one present in VS Code and re-use some already existing localizations.

I believe that it is incorrect to default to the "Quit Without Saving" button which is selected by default. Pressing Enter should probably Save All instead similarly to VS Code.


Please also be sure to properly fill out the pull-request description with "How To Test", for example I don't believe these changes apply to the browser as you have them today?

packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
@emilhammarstedtst emilhammarstedtst force-pushed the pr/save-changes-on-quit branch 2 times, most recently from 1361177 to db1b421 Compare February 9, 2023 14:55
@emilhammarstedtst
Copy link
Contributor Author

@emilhammarstedtst thank you for the contribution +1

I have some general comments about the dialog, mainly to align with the one present in VS Code and re-use some already existing localizations.

I believe that it is incorrect to default to the "Quit Without Saving" button which is selected by default. Pressing Enter should probably Save All instead similarly to VS Code.

Please also be sure to properly fill out the pull-request description with "How To Test", for example I don't believe these changes apply to the browser as you have them today?

Amended the commit with your suggestions and cleared up the "undefined"-leftover.
However the title now potentially has the the text " following 0 files?", so i created a fallback case for this.

I dont believe the browser is in question as it is bound to use the onbeforeunload method?

@emilhammarstedtst emilhammarstedtst force-pushed the pr/save-changes-on-quit branch 2 times, most recently from dce369c to e7ea5ea Compare February 10, 2023 16:08
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.

Looks good to me 👍 I have 2 minor comments before merging though.

packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
- Question to save all changes, cancel or discard all changes before closing
- Display what is not yet saved.
- Reorder options and reword the question and options to be clear of the outcome of each action
- Implemented using ConfirmDialogSave extending ConfirmDialog

Contributed by STMicroelectronics

Signed-off-by: Emil Hammarstedt <[email protected]>
@vince-fugnitto
Copy link
Member

@msujew do we want to merge for today's release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core issues related to the core of the application
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants