Add support for "preferred" AlertButton #32538

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: facebook/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

Libraries/Alert/Alert.js

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

Libraries/Alert/Alert.js

@@ -122,6 +124,8 @@ class Alert {
            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 facebook#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 facebook/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 15bc6c5 Compare November 4, 2021 10:00

ken0nek reviewed Nov 4, 2021

View reviewed changes

Libraries/Alert/Alert.js Outdated Show resolved Hide resolved

robbie-c force-pushed the main branch from 0f342c4 to a6b3010 Compare 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 91728e2 Compare 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 4394989 Compare November 11, 2021 13:11

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

danilobuerger reviewed Feb 11, 2022

View reviewed changes

lunaleaps requested 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 000bbe8 Jul 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 facebook#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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for "preferred" AlertButton #32538

Add support for "preferred" AlertButton #32538

robbie-c commented Nov 4, 2021 •

edited

Loading

analysis-bot commented Nov 4, 2021 •

edited

Loading

analysis-bot commented Nov 4, 2021 •

edited

Loading

ken0nek left a comment

robbie-c commented Nov 10, 2021

robbie-c commented Nov 11, 2021

robbie-c commented Nov 17, 2021

ken0nek commented Nov 19, 2021

danilobuerger commented Feb 11, 2022

danilobuerger Feb 11, 2022

robbie-c Mar 19, 2022

facebook-github-bot commented Feb 19, 2022

danilobuerger commented Feb 28, 2022

lunaleaps left a comment

robbie-c commented Mar 19, 2022 •

edited

Loading

lunaleaps commented Mar 25, 2022 •

edited

Loading

robbie-c commented Apr 19, 2022

ken0nek commented Jun 28, 2022

react-native-bot commented Jul 22, 2022

Add support for "preferred" AlertButton #32538

Add support for "preferred" AlertButton #32538

Conversation

robbie-c commented Nov 4, 2021 • edited Loading

Summary

Changelog

Test Plan

analysis-bot commented Nov 4, 2021 • edited Loading

analysis-bot commented Nov 4, 2021 • edited Loading

ken0nek left a comment

Choose a reason for hiding this comment

robbie-c commented Nov 10, 2021

robbie-c commented Nov 11, 2021

robbie-c commented Nov 17, 2021

ken0nek commented Nov 19, 2021

danilobuerger commented Feb 11, 2022

danilobuerger Feb 11, 2022

Choose a reason for hiding this comment

robbie-c Mar 19, 2022

Choose a reason for hiding this comment

facebook-github-bot commented Feb 19, 2022

danilobuerger commented Feb 28, 2022

lunaleaps left a comment

Choose a reason for hiding this comment

robbie-c commented Mar 19, 2022 • edited Loading

lunaleaps commented Mar 25, 2022 • edited Loading

robbie-c commented Apr 19, 2022

ken0nek commented Jun 28, 2022

react-native-bot commented Jul 22, 2022

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