pankod / react-native-picker-modal-view

An unified React Native Picker Modal component for iOS and Android.
204 stars 55 forks source link

Allow onSelected to accept React.useState callbacks #68

Open IlyaSemenov opened 3 years ago

IlyaSemenov commented 3 years ago

Most React components that deal with user input accept callbacks returned by React.useState, for instance:

import { TextInput } from "react-native"

...

export const MyComponent = () => {
  const [value, setValue] = React.useState("")
  return <TextInput value={value} onChangeText={setValue} />
}

This is however not the case with your component's onSelected. One has to write boilerplate like:

<PickerModal onSelected={(object) => {
    setValue("" + object.Id) // object.Id is not text but React.ReactText?? one more weirdness.
    return object
}} />

This API is awkward and not following React best practices. There's no point in returning anything. Like: why would you want to pass a (possibly updated) object further? If you need it for renderSelectView, then there is already prop selected for that. Normally, in React, it's up to onSelected to update (or don't update) some external variable which is then passed to selected as a prop. Your component should not be managing the state itself.

Please consider updating the API to how literally every other similar component works. At the very least, allow callbacks to return nothing (undefined) because currently it's not the case.

issue-label-bot[bot] commented 3 years ago

Issue-Label Bot is automatically applying the label feature_request to this issue, with a confidence of 0.88. Please mark this comment with :thumbsup: or :thumbsdown: to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

omeraplak commented 3 years ago

@IlyaSemenov Hi, You are right. We want to rewrite this component with hooks. Thanks for your feedback.

IlyaSemenov commented 3 years ago

Just to make it clear, the component doesn't necessary have to be rewritten with hooks. It works just fine as a class component. The problem is different: the component apparently stores the internal state (what has been selected), and updates it with the result of onSelected callback. Instead, it should simply use selected property for the current state, so that the state is managed by external code (not the component).

Of course it means breaking the API, so if you decide to follow this road that could land in a next major release.