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

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

Merged
merged 1 commit into from Sep 23, 2021

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Aug 10, 2021

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 in the last 24 hours 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
Copy link
Member

@codebytere codebytere left a comment

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
Copy link
Contributor Author

miniak commented Aug 11, 2021

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.

Copy link
Member

@zcbenz zcbenz left a comment

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.

shell/browser/ui/message_box_mac.mm Outdated Show resolved Hide resolved
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 17, 2021
@miniak miniak requested review from a team as code owners August 18, 2021 20:38
@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
Copy link

release-clerk bot commented Sep 23, 2021

Release Notes Persisted

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

@trop
Copy link
Contributor

trop bot commented Sep 23, 2021

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

@trop
Copy link
Contributor

trop bot commented Sep 23, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review/approved ✅ semver/minor backwards-compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants