Skip to content

feat: add textWidth option to dialog.showMessageBox() - #30474

Merged
zcbenz merged 1 commit into
mainfrom
miniak/message-box-width
Sep 23, 2021
Merged

feat: add textWidth option to dialog.showMessageBox()#30474
zcbenz merged 1 commit into
mainfrom
miniak/message-box-width

Conversation

@miniak

@miniak miniak commented Aug 10, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description of Change

This allows working around the overly narrow NSAlert dialogs on macOS Big Sur. Default:
Screen Shot 2021-08-10 at 8 50 33 PM

textWidth = 420:
Screen Shot 2021-08-10 at 8 51 24 PM

Checklist

Release Notes

Notes: Added textWidth option to dialog.showMessageBox() / dialog.showMessageBoxSync().

@electron-cation electron-cation Bot added the new-pr 🌱 PR opened recently label Aug 10, 2021
@miniak miniak self-assigned this Aug 10, 2021
@miniak miniak added semver/minor backwards-compatible functionality target/15-x-y labels Aug 10, 2021
@miniak
miniak force-pushed the miniak/message-box-width branch 2 times, most recently from e13bf38 to 67f67ba Compare August 10, 2021 20:03
@miniak miniak changed the title feat: add width option to dialog.showMessageBox() feat: add textWidth option to dialog.showMessageBox() Aug 10, 2021
codebytere
codebytere reviewed Aug 11, 2021
View reviewed changes

@codebytere codebytere left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree this is a problem worth addressing, but my mild concern here is that this solution is a bit narrow and might prevent us from addressing other similar issues in a more robust way in the future.

For example - with this users can specify a custom width, but would they ever experience issues with height? Would it perhaps make more sense to specify x, y, width, height as a gfx::Rect here? If that's a potential i'd definitely want to avoid adding more one-off properties than necessary 🤔

@miniak

miniak commented Aug 11, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I agree this is a problem worth addressing, but my mild concern here is that this solution is a bit narrow and might prevent us from addressing other similar issues in a more robust way in the future.

For example - with this users can specify a custom width, but would they ever experience issues with height? Would it perhaps make more sense to specify x, y, width, height as a gfx::Rect here?

@codebytere It's only possible to specify width. I am using a dummy https://developer.apple.com/documentation/appkit/nsalert/1530575-accessoryview?language=objc, which has specified width and zero height. There does not seem to be any other good way to make the dialog wider.

zcbenz
zcbenz suggested changes Aug 13, 2021
View reviewed changes

@zcbenz zcbenz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The API looks good to me.

Comment thread shell/browser/ui/message_box_mac.mm Resolved
@electron-cation electron-cation Bot removed the new-pr 🌱 PR opened recently label Aug 17, 2021
@miniak
miniak requested review from a team as code owners August 18, 2021 20:38
@miniak
miniak force-pushed the miniak/message-box-width branch from f8dbc11 to 114bf2c Compare August 18, 2021 20:57
@miniak
miniak requested review from zcbenz and removed request for a team August 18, 2021 20:58
@zcbenz
zcbenz merged commit 7757961 into main Sep 23, 2021
@zcbenz
zcbenz deleted the miniak/message-box-width branch September 23, 2021 10:56
@release-clerk

release-clerk Bot commented Sep 23, 2021

Copy link
Copy Markdown

Release Notes Persisted

Added textWidth option to dialog.showMessageBox() / dialog.showMessageBoxSync().

@trop trop Bot mentioned this pull request Sep 23, 2021
Merged
@trop trop Bot mentioned this pull request Sep 23, 2021
Merged
@trop

trop Bot commented Sep 23, 2021

Copy link
Copy Markdown
Contributor

I have automatically backported this PR to "15-x-y", please check out #31088

@trop

trop Bot commented Sep 23, 2021

Copy link
Copy Markdown
Contributor

I have automatically backported this PR to "16-x-y", please check out #31089

@ghost ghost mentioned this pull request Dec 15, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@codebytere codebytere codebytere approved these changes
+1 more reviewer
@zcbenz zcbenz zcbenz approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@miniak miniak

Labels

api-review/approved ✅ semver/minor backwards-compatible functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@miniak @zcbenz @codebytere @jkleinsc