Add support for "preferred" AlertButton by robbie-c · Pull Request #32538 · react/react-native

robbie-c · 2021-11-04T09:49:47Z

Summary

Currently, with the Alert API on iOS, the only way to bold one of the buttons is by setting the style to "cancel". This has the side-effect of moving it to the left. The underlying UIKit API has a way of setting a "preferred" button, which does not have this negative side-effect, so this PR wires this up.

See preferredAction on UIAlertController https://developer.apple.com/documentation/uikit/uialertcontroller/

Docs PR: react/react-native-website#2839

Changelog

[iOS] [Added] - Support setting an Alert button as "preferred", to emphasize it without needing to set it as a "cancel" button.

Test Plan

I ran the RNTesterPods app and added an example. It has a button styled with "preferred" and another with "cancel", to demonstrate that the "preferred" button takes emphasis over the "cancel" button.

analysis-bot · 2021-11-04T10:30:44Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 91728e2
Branch: main

analysis-bot · 2021-11-04T10:48:04Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,478,011	+292
android	hermes	armeabi-v7a	7,775,258	+293
android	hermes	x86	8,947,150	+284
android	hermes	x86_64	8,894,079	+294
android	jsc	arm64-v8a	9,791,896	+116
android	jsc	armeabi-v7a	8,752,619	+106
android	jsc	x86	9,740,725	+117
android	jsc	x86_64	10,341,586	+117

Base commit: 91728e2
Branch: main

ken0nek

It seems I have no permission to Approve but LGTM. Thank you!

robbie-c · 2021-11-10T21:32:11Z

I don't believe the test failures were introduced in this PR - none of the failures in analyze_code were things touched by me, build_npm_package looks like a config problem, I'm less confident about test_ios_unit_hermes but it doesn't look related.

See preferredAction on UIAlertController https://developer.apple.com/documentation/uikit/uialertcontroller/

robbie-c · 2021-11-11T13:13:51Z

Just rebased this to the most recent commit where CI passed

robbie-c · 2021-11-17T11:33:56Z

Is there anything I can do to help get this merged?

ken0nek · 2021-11-19T02:11:37Z

@robbie-c I think you are ready. We just need approval from React Native Team. I don't know how to get attention though...

danilobuerger · 2022-02-11T12:48:21Z

@cortinico Hi, would it be possible to get this reviewed by the team?

danilobuerger · 2022-02-11T13:10:55Z

 cancelButtonKey = String(index);
 } else if (btn.style === 'destructive') {
 destructiveButtonKey = String(index);
+ } else if (btn.isPreferred) {


@robbie-c shouldn't this just be an if? It shouldn't depend on the style?

That's fair, whilst preferred and destructive seem mutually exclusive visually (it's been a while since I wrote this so I don't remember which one "wins" if you apply both), if we're just passing through flags to the iOS API in an unopionated way it makes sense to match it, so I'll make this change.

facebook-github-bot · 2022-02-19T00:10:06Z

@lunaleaps has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

danilobuerger · 2022-02-28T19:00:51Z

Hi @robbie-c, did you see my review comment?

lunaleaps

@robbie-c Thanks for adding this feature! For this change, we'll need to add a new method call for the new parameter to ensure we don't break for apps that use OTA. We'll need to conditionally check that the new native method exists on JS as well

robbie-c · 2022-03-19T10:37:53Z

Apologies @danilobuerger @lunaleaps , I'm just seeing these comments now.

@lunaleaps just to clarify what you mean - are you talking about adding a new native method (something like alertWithArgs2) that we'd call if it exists, but leave the original native iOS alertWithArgs unchanged for backwards compatibility.

What's the guarantees provided with OTA? Is it that new JS code always works with old native, or does it also need to hold true in the other direction as well. I'm just trying to judge how hard it would be to keep one alertWithArgs and make it compatible.

lunaleaps · 2022-03-25T22:01:20Z

@robbie-c Yea can we add a new method with the new argument into the native module (turbo module) and type it as nullable. New JS needs to work with old native and only in that direction. In JS usage, you'd need also need to do a null check before using; also for the case of Android.

robbie-c · 2022-04-19T20:08:12Z

Apologies for the delays on this - the day job is pretty busy and I'm doing 70-90 hour weeks most weeks so I'm not finding a lot of time for things like this

ken0nek · 2022-06-28T21:48:14Z

@lunaleaps @ryancat I have one question.

we'll need to add a new method call for the new parameter to ensure we don't break for apps that use OTA.

What is the fundamental difference between #33553 and this pull request?
It seems #33553 changes the native code and has no new method but it's already merged.

react-native-bot · 2022-07-22T22:45:04Z

This pull request was successfully merged by @robbie-c in 000bbe8.

^{When will my fix make it into a release? | Upcoming Releases}

See react#32538 for the discussion

Summary: See #32538 for the discussion cc robbie-c ## Changelog  [iOS] [Fixed] - Allow preferred Alert button regardless of the style Pull Request resolved: #34253 Test Plan: Same test plan as PR mentioned above Reviewed By: lunaleaps Differential Revision: D38112619 Pulled By: cipolleschi fbshipit-source-id: 2ac4fc6226a859e69f0df27913898effa5e092eb

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Nov 4, 2021

robbie-c mentioned this pull request Nov 4, 2021
Add the "preferred" AlertButtonStyle react/react-native-website#2839
Merged

react-native-bot added Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Nov 4, 2021

robbie-c force-pushed the main branch from 4fd031f to 15bc6c5Compare November 4, 2021 10:00

ken0nek reviewed Nov 4, 2021
View reviewed changes

Comment threadLibraries/Alert/Alert.js Outdated

robbie-c force-pushed the main branch from 0f342c4 to a6b3010Compare November 10, 2021 15:37

ken0nek reviewed Nov 10, 2021
View reviewed changes

robbie-c closed this Nov 11, 2021

robbie-c force-pushed the main branch from 07fb206 to 91728e2Compare November 11, 2021 13:03

robbie-c reopened this Nov 11, 2021

robbie-c added 2 commits November 11, 2021 13:11

Add support for "preferred" AlertButtonStyle
48c052d
See preferredAction on UIAlertController https://developer.apple.com/documentation/uikit/uialertcontroller/

Moved "preferred" AlertButtonStyle to a separate flag "isPreferred"
4394989

robbie-c force-pushed the main branch from e4b7058 to 4394989Compare November 11, 2021 13:11

robbie-c changed the title ~~Add support for "preferred" AlertButtonStyle~~Add support for "preferred" AlertButtonNov 15, 2021

danilobuerger reviewed Feb 11, 2022
View reviewed changes

lunaleaps suggested changes Mar 10, 2022
View reviewed changes

ken0nek mentioned this pull request Apr 5, 2022
Added userInterfaceStyle to Alert to override user interface style for iOS 13+ #33553
Closed

facebook-github-bot closed this in 000bbe8Jul 22, 2022

react-native-bot added the Merged This PR has been merged. label Jul 22, 2022

danilobuerger added a commit to danilobuerger/react-native that referenced this pull request Jul 23, 2022
Allow preferred Alert button regardless of the style
52b0de0
See react#32538 for the discussion

This was referenced Jul 23, 2022
Allow preferred Alert button regardless of the style robbie-c/react-native#1
Closed
Allow preferred Alert button regardless of the style #34253
Closed

Uh oh!

Conversation

robbie-c commented Nov 4, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changelog

Test Plan

Uh oh!

analysis-bot commented Nov 4, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analysis-bot commented Nov 4, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ken0nek left a comment

Choose a reason for hiding this comment

Uh oh!

robbie-c commented Nov 10, 2021

Uh oh!

robbie-c commented Nov 11, 2021

Uh oh!

robbie-c commented Nov 17, 2021

Uh oh!

ken0nek commented Nov 19, 2021

Uh oh!

danilobuerger commented Feb 11, 2022

Uh oh!

danilobuergerFeb 11, 2022

Choose a reason for hiding this comment

Uh oh!

robbie-cMar 19, 2022

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 19, 2022

Uh oh!

danilobuerger commented Feb 28, 2022

Uh oh!

lunaleaps left a comment

Choose a reason for hiding this comment

Uh oh!

robbie-c commented Mar 19, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lunaleaps commented Mar 25, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robbie-c commented Apr 19, 2022

Uh oh!

ken0nek commented Jun 28, 2022

Uh oh!

react-native-bot commented Jul 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

robbie-c commented Nov 4, 2021•
edited
Loading

analysis-bot commented Nov 4, 2021•
edited
Loading

analysis-bot commented Nov 4, 2021•
edited
Loading

robbie-c commented Mar 19, 2022•
edited
Loading

lunaleaps commented Mar 25, 2022•
edited
Loading