feat: add textWidth option to dialog.showMessageBox() by miniak · Pull Request #30474 · electron/electron · GitHub
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

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 Outdated
@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

@commit-lint commit-lint Bot 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