Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Set electron dialogs to modal by default #13043

Merged
merged 3 commits into from
Nov 10, 2023

Conversation

zole40
Copy link
Contributor

@zole40 zole40 commented Nov 1, 2023

What it does

I found regression in related to the electron dialogs #13042

How to test

  1. Build the electron example
  2. Run electron example
  3. Go to the file menu and click on the open folder

Follow-ups

Review checklist

Reminder for reviewers

Signed-off-by: Zoltán Béres <zole40@gmail.com>
@zole40 zole40 force-pushed the electron_modal_dialog branch from dd645b0 to 12bb9c3 Compare November 1, 2023 09:42
Copy link
Contributor

@AlexandraBuzila AlexandraBuzila left a comment

Choose a reason for hiding this comment

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

Hi @zole40! Thank you for the fix, it works as expected 👍

However, I think it would be better if the modal prop was already initialized in the toOpenDialogOptions and toSaveDialogOptions from the ElectronFileDialogService, together with the other dialog options.

@zole40
Copy link
Contributor Author

zole40 commented Nov 8, 2023

Hi @zole40! Thank you for the fix, it works as expected 👍

However, I think it would be better if the modal prop was already initialized in the toOpenDialogOptions and toSaveDialogOptions from the ElectronFileDialogService, together with the other dialog options.

Should we set a default value if it is undefined?
For example: result.modal = props.modal ?? true;

@AlexandraBuzila
Copy link
Contributor

Should we set a default value if it is undefined? For example: result.modal = props.modal ?? true;

Yes, exactly. That would ensure that the options already have the default value for the modal prop when it is checked in electron-api-main.ts

Signed-off-by: Zoltán Béres <zole40@gmail.com>
Signed-off-by: Zoltán Béres <zole40@gmail.com>
Copy link
Contributor

@AlexandraBuzila AlexandraBuzila left a comment

Choose a reason for hiding this comment

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

Thank you, it looks good to me!

@JonasHelming could you have a look as well, please?

@JonasHelming JonasHelming requested a review from tsmaeder November 9, 2023 10:19
@tsmaeder tsmaeder merged commit 5fcb2f5 into eclipse-theia:master Nov 10, 2023
@zole40 zole40 deleted the electron_modal_dialog branch November 10, 2023 09:29
@vince-fugnitto vince-fugnitto added this to the 1.44.0 milestone Nov 30, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants