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

Allow onSelected to accept React.useState callbacks #68

Open
IlyaSemenov opened this issue Oct 20, 2020 · 3 comments
Open

Allow onSelected to accept React.useState callbacks #68

IlyaSemenov opened this issue Oct 20, 2020 · 3 comments

Comments

@IlyaSemenov
Copy link

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
Copy link

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

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

@omeraplak
Copy link
Contributor

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

@IlyaSemenov
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants