ActionSheetIOS add destructiveButtonIndexes array #13924

sdg9 · 2017-05-11T17:40:52Z

Motivation (required)

Make destructive choices prominent. Use red for buttons that perform destructive or dangerous actions, and display these buttons at the top of an action sheet.

Currently ActionSheetIOS's showActionSheetWithOptions only supports a single destructive button via the prop destructiveButtonIndex.

This PR maintains backwards compatibility with destructiveButtonIndex while simultaneously supporting destructiveButtonIndexes allowing developers to pass an array of destructive indexes

Test Plan (required)

ActionSheetIOS.showActionSheetWithOptions({
  options: ['one', 'two', 'three'],
  destructiveButtonIndexes: [0, 1],
}, () => {});

Some additional tests, all working as expected (item only red if it is a matching index).

    ActionSheetIOS.showActionSheetWithOptions({
      options: ['one', 'two', 'three'],
      destructiveButtonIndexes: [0, 19],
    }, () => {});

    ActionSheetIOS.showActionSheetWithOptions({
      options: ['one', 'two', 'three'],
      destructiveButtonIndexes: [],
    }, () => {});

    ActionSheetIOS.showActionSheetWithOptions({
      options: ['one', 'two', 'three'],
      destructiveButtonIndexes: undefined,
    }, () => {});

    ActionSheetIOS.showActionSheetWithOptions({
      options: ['one', 'two', 'three'],
    }, () => {});

    ActionSheetIOS.showActionSheetWithOptions({
      options: ['one', 'two', 'three'],
      destructiveButtonIndexes: [0, 5, 0, 0],
    }, () => {});

    ActionSheetIOS.showActionSheetWithOptions({
      options: ['one', 'two', 'three'],
      destructiveButtonIndexes: [0, 5, 0, 0, 'non numeric', 12.34],
    }, () => {});

The following will crash the app but I believe this is expected

    ActionSheetIOS.showActionSheetWithOptions({
      options: ['one', 'two', 'three'],
      destructiveButtonIndexes: 'not an array',
    }, () => {});

    ActionSheetIOS.showActionSheetWithOptions({
      options: ['one', 'two', 'three'],
      destructiveButtonIndexes: null,
    }, () => {});

Explain the motivation for making this change.
Provide a test plan demonstrating that the code is solid.
Match the code formatting of the rest of the codebase.
Target the master branch, NOT a "stable" branch.

janicduplessis · 2017-06-06T08:13:39Z

Thanks for the PR!

I think we should avoid having 2 options to do the same thing. I can think of 2 possibilities; allow passing both int and Array<int> to destructiveButtonIndex or add destructiveButtonIndices (We seem to use indices over indexes in the codebase) and deprecate destructiveButtonIndex.

I'm leaning a bit for the first solution since we won't have to deprecate anything and even if it isn't the best naming probably that 99% of the time people will want only 1 destructive button.

sdg9 · 2017-06-07T18:23:54Z

To avoid two properties doing similar things I agree your first suggestion is the more pragmatic approach.

Can I get confirmation that overloading destructiveButtonIndex to accept an int or array of ints is the desired approach before I update the code?

janicduplessis · 2017-06-12T19:38:12Z

Yes this is fine! Thanks

facebook-github-bot · 2017-08-30T17:02:20Z

@sdg9 I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

…ButtonIndexes Overload destructiveButtonIndex for int and array

sdg9 · 2017-10-11T12:25:08Z

@janicduplessis sorry for the delay, thanks for the help @noahmalmed

facebook-github-bot · 2017-11-10T18:04:12Z

@sdg9 I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

stale · 2018-01-09T19:32:18Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

Summary: This is a recreation of #13924, rebased on top of master, as the former PR wasn't re-reviewed and automatically closed by the bot. iOS [Action Sheets docs](https://developer.apple.com/ios/human-interface-guidelines/ui-views/action-sheets/) say > Make destructive choices prominent. Use red for buttons that perform destructive or dangerous actions, and display these buttons at the top of an action sheet. Currently ActionSheetIOS's showActionSheetWithOptions only supports a single destructive button via the prop `destructiveButtonIndex`. This PR maintains backwards compatibility with `destructiveButtonIndex` while simultaneously supporting `destructiveButtonIndexes` allowing developers to pass an array of destructive indexes ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: [0, 1], }, () => {}); ``` <img width="282" alt="actionsheet" src="https://cloud.githubusercontent.com/assets/3091143/25963211/1c211a16-3646-11e7-9b7c-c9a2dbea7832.png"> Some additional tests, all working as expected (item only red if it is a matching index). ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: [0, 19], }, () => {}); ``` ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: [], }, () => {}); ``` ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: undefined, }, () => {}); ``` ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], }, () => {}); ``` ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: [0, 5, 0, 0], }, () => {}); ``` ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: [0, 5, 0, 0, 'non numeric', 12.34], }, () => {}); ``` The following will crash the app but I believe this is expected ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: 'not an array', }, () => {}); ``` ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: null, }, () => {}); ``` - [x] Explain the **motivation** for making this change. - [x] Provide a **test plan** demonstrating that the code is solid. - [x] Match the **code formatting** of the rest of the codebase. - [x] Target the `master` branch, NOT a "stable" branch. Pull Request resolved: #18254 Differential Revision: D13680516 Pulled By: hramos fbshipit-source-id: ac183cdcf5e1daef8e3c584dcf6a921bbecad475

Summary: This is a recreation of facebook#13924, rebased on top of master, as the former PR wasn't re-reviewed and automatically closed by the bot. iOS [Action Sheets docs](https://developer.apple.com/ios/human-interface-guidelines/ui-views/action-sheets/) say > Make destructive choices prominent. Use red for buttons that perform destructive or dangerous actions, and display these buttons at the top of an action sheet. Currently ActionSheetIOS's showActionSheetWithOptions only supports a single destructive button via the prop `destructiveButtonIndex`. This PR maintains backwards compatibility with `destructiveButtonIndex` while simultaneously supporting `destructiveButtonIndexes` allowing developers to pass an array of destructive indexes ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: [0, 1], }, () => {}); ``` <img width="282" alt="actionsheet" src="https://cloud.githubusercontent.com/assets/3091143/25963211/1c211a16-3646-11e7-9b7c-c9a2dbea7832.png"> Some additional tests, all working as expected (item only red if it is a matching index). ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: [0, 19], }, () => {}); ``` ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: [], }, () => {}); ``` ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: undefined, }, () => {}); ``` ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], }, () => {}); ``` ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: [0, 5, 0, 0], }, () => {}); ``` ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: [0, 5, 0, 0, 'non numeric', 12.34], }, () => {}); ``` The following will crash the app but I believe this is expected ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: 'not an array', }, () => {}); ``` ```js ActionSheetIOS.showActionSheetWithOptions({ options: ['one', 'two', 'three'], destructiveButtonIndexes: null, }, () => {}); ``` - [x] Explain the **motivation** for making this change. - [x] Provide a **test plan** demonstrating that the code is solid. - [x] Match the **code formatting** of the rest of the codebase. - [x] Target the `master` branch, NOT a "stable" branch. Pull Request resolved: facebook#18254 Differential Revision: D13680516 Pulled By: hramos fbshipit-source-id: ac183cdcf5e1daef8e3c584dcf6a921bbecad475

ken0x0a · 2019-04-15T02:35:20Z

plz merge this I want to use.
even Instagram using multiple destructive button.
Why not?

janicduplessis · 2019-04-19T05:38:06Z

@ken0x0a This was merged in #18254

ken0x0a · 2019-04-19T10:32:48Z

@janicduplessis Thank you for your reply!

I forget that I wrote here.
Actually I found this was merged, but even though I can not use because expo/react-native still not merge #18254.

I'm now waiting for expo merge this.

Add support for an array of destructiveButtonIndexes

74fc165

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. GH Review: review-needed labels May 11, 2017

Steven Goff and others added 2 commits May 11, 2017 12:57

Fix missing { typo

6c694c1

Merge branch 'master' into feature/actionSheet-destructiveButtonIndexes

b73cf24

shergin added the JavaScript label May 28, 2017

Noah Malmed and others added 2 commits October 10, 2017 13:10

Overload destructiveButtonIndex for int and array

e524281

Merge pull request #1 from noahmalmed/feature/actionSheet-destructive…

eef7006

…ButtonIndexes Overload destructiveButtonIndex for int and array

stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jan 9, 2018

stale bot closed this Jan 16, 2018

sdg9 mentioned this pull request Mar 7, 2018

Feature/action sheet destructive button indexes #18254

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ActionSheetIOS add destructiveButtonIndexes array #13924

ActionSheetIOS add destructiveButtonIndexes array #13924

sdg9 commented May 11, 2017 •

edited

Loading

janicduplessis commented Jun 6, 2017

sdg9 commented Jun 7, 2017

janicduplessis commented Jun 12, 2017

facebook-github-bot commented Aug 30, 2017

sdg9 commented Oct 11, 2017

facebook-github-bot commented Nov 10, 2017

stale bot commented Jan 9, 2018

ken0x0a commented Apr 15, 2019

janicduplessis commented Apr 19, 2019

ken0x0a commented Apr 19, 2019

ActionSheetIOS add destructiveButtonIndexes array #13924

ActionSheetIOS add destructiveButtonIndexes array #13924

Conversation

sdg9 commented May 11, 2017 • edited Loading

Motivation (required)

Test Plan (required)

janicduplessis commented Jun 6, 2017

sdg9 commented Jun 7, 2017

janicduplessis commented Jun 12, 2017

facebook-github-bot commented Aug 30, 2017

sdg9 commented Oct 11, 2017

facebook-github-bot commented Nov 10, 2017

stale bot commented Jan 9, 2018

ken0x0a commented Apr 15, 2019

janicduplessis commented Apr 19, 2019

ken0x0a commented Apr 19, 2019

sdg9 commented May 11, 2017 •

edited

Loading