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

Declarative ActionSheetIOS API #10

Closed
maxaggedon opened this issue Aug 8, 2018 · 9 comments
Closed

Declarative ActionSheetIOS API #10

maxaggedon opened this issue Aug 8, 2018 · 9 comments
Labels
🗣 Discussion This label identifies an ongoing discussion on a subject

Comments

@maxaggedon
Copy link

maxaggedon commented Aug 8, 2018

I find it painful to maintain an ActionSheet with entries that can dynamically change.

I would like to propose a change of the showActionSheetWithOptions() signature so that the cancelButtonIndex, the destructiveButtonIndex and the buttons callbacks would be contained in the options array alongside the labels.

showActionSheetWithOptions({
  title: "My ActionSheet",
  options: [
    {
      label: "First Button",
      onPress: () => console.log("First Button pressed")
    },
    {
      label: "Cancel",
      onPress: () => console.log("Canceled"),
      cancel: true
    },
    {
      label: "Delete",
      onPress: () => console.log("Deleted"),
      destructive: true
    },
  ]
})

The goal is that these indices would be taken care of by the library and not the user. It would then be simple to add or remove a button from the options. I did implement this like that:

 showActionSheetWithOptions(
  {
    options: options.map(o => o.label),
    title: this.project.name,
    cancelButtonIndex: options.findIndex(o => o.cancel),
    destructiveButtonIndex: options.findIndex(o => o.destructive),
  },
  index => {
    options[index].onPress && options[index].onPress();
  }
)

but I think it could be done directly in React Native.

I think this is the place to discuss such changes, if not please tell me where :)

@kelset kelset added the 🗣 Discussion This label identifies an ongoing discussion on a subject label Aug 8, 2018
@kelset
Copy link
Member

kelset commented Aug 8, 2018

I think it's fine here, can I ask you to edit your issue and add a link to the wrapper you mention here

I did implement this as a wrapper around the current api

since I think it could help :)

@maxaggedon
Copy link
Author

@kelset I just inserted it in the original issue. It is actually very simple and the only loss I can see is that there is no global callback, but it could be added.

@axe-fb
Copy link
Collaborator

axe-fb commented Aug 8, 2018

@maxaggedon - In the spirit of the slimmening, i wanted to see if we can move this component outside React Native Core in the first place. Also given that this is iOS only, I would want to see if we can have an "ActionSheet" for both iOS and Android. Would it make sense to fork/copy the code for ActionSheetIOS and then add this ? This way, this may be an extra feature that is added, and will not impact core. Like with the webview, we could start off with a proposal on how we can separate it from core, and then continue independently developing this component ?

@maxaggedon
Copy link
Author

@axe-fb

I would want to see if we can have an "ActionSheet" for both iOS and Android.

As of now I am using @expo/react-native-action-sheet, which provides a pure javascript Android action sheet, with the same API. It could be the missing part for the new cross-platform ActionSheet, don't you think ?

Would it make sense to fork/copy the code for ActionSheetIOS and then add this ? [...] Like with the webview, we could start off with a proposal on how we can separate it from core, and then continue independently developing this component ?

I guess so. I have not yet contributed to react-native so I don't really know what to think. But I guess this is the way to go, considering the slimmening is on its way. Can you start the proposal ?

@axe-fb
Copy link
Collaborator

axe-fb commented Aug 10, 2018

Given that expo already has a react-native-action-sheet, i am inclined to make this lower priority. We may simply decide to remove ActionSheet-iOS from the core, and direct people to expo's solution.

Does it make sense to close this issue for now, and take another look at it when we work on deprecating some of the core components ?

@maxaggedon
Copy link
Author

Actually the expo repo uses ActionSheetIOS from the core, so instead of just removing the component from the core, maybe it should be extract it there. Is that what you suggest ?

Regarding the origin of this issue, the expo repo uses the same api as the core, and I had already planned to suggest the same over there.

Maybe we should open an issue there about extracting ActionSheetIOS and I will open another one regarding the api ?

Let me know what you think.

@eliperkins
Copy link

I've thought about a declarative API for ActionSheet that is similar to <Modal />, using the visible prop, so something like <ActionSheet visible={showingActionSheet} />.

In an ideal world, I'd love an API like:

<ActionSheet
  visible={false}
  title="Erase Contents and Settings"
  message="Are you sure you want to erase your phone?"
  tintColor="red"
>
  <ActionSheet.Destructive onAction={this.onErase}>
    Erase Phone
  </ActionSheet.Destructive>
  <ActionSheet.Action onAction={this.onReadMore}>Read More</ActionSheet.Action>
  <ActionSheet.Cancel onAction={this.onCancel}>Cancel</ActionSheet.Cancel>
</ActionSheet>;

I'm pretty sure this is feasible, but it might be tough to contain it to a specific view (controller), which can be necessary in some cases.

@axe-fb
Copy link
Collaborator

axe-fb commented Aug 28, 2018

Closing this issue as we discussed that we will not be looking to fix this. Please re=open if you think that we should still talk about it.

@axe-fb
Copy link
Collaborator

axe-fb commented Aug 28, 2018

Duh, looks like i cannot close issues. Summoning @grabbou to help close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗣 Discussion This label identifies an ongoing discussion on a subject
Projects
None yet
Development

No branches or pull requests

4 participants