-
Notifications
You must be signed in to change notification settings - Fork 280
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
Update Prune dialog to current design #2138
Conversation
Expose the message box API to the renderer (but not extensions) and switch the Prune dialog to use it. This changes the prune dialog to match current design guidelines. Although we may still need to update the Modal component to the new design, this simplifies the prune code as well. Partial fix for issue containers#1987. Signed-off-by: Tim deBoer <git@tdeboer.ca>
Signed-off-by: Tim deBoer <git@tdeboer.ca>
message += ' from the ' + engines[0].name + ' engine.'; | ||
} | ||
|
||
const result = await window.showMessageBox({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering we should not align with the extension API that exposes showInformationMessage
, showWarningMessage
and showErrorMessage
rather that showMessageBox
which is lower level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message box API is slightly lower level, but it only requires one method instead of 4 variants (info/warn/error/question) and using properties is much more extendable. e.g. we could continue to add properties similar to Electron API (https://www.electronjs.org/docs/latest/api/dialog) and allow detailed messages, checkbox, assign default/cancel buttons, etc. For instance our old telemetry dialog would have required message box b/c it had a checkbox.
If you'd still prefer I can just expose a showQuestionBox (or all 4 variants) for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you convinced me your choose the right path but maybe we should extend the extension API to have the same level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put that in the original MessageBox PR and Florent (correctly) pointed out I was doing too much in one PR and exposing API when we didn't really have a need for it yet. I think it's probably better to wait until we have a request (?) but I'm happy to do a separate PR for that whenever.
What does this PR do?
Expose the message box API to the renderer (but not extensions) and switch the Prune dialog to use it. This changes the prune dialog to match current design guidelines.
Although we may still need to update the Modal component to the new design, this simplifies the prune code as well.
Screenshot/screencast of this PR
Before:
After:
What issues does this PR fix or reference?
Partial fix for issue #1987.
How to test this PR?
Prune! Try 'no', 'yes' and cancelling the dialog.