Picker (android): Convert children to an array before accessing with a position #8153

justim · 2016-06-16T15:11:32Z

When using the following component, this.props.children is not a flat array.

class Example extends Component {
    // ...

    render() {
        const values = ['1', '2'];

        return (
            <Picker
                value={this.state.value}
                onValueChange={this.onValueChange.bind(this)}
            >
                <Picker.Item
                    label="n/a"
                    value={null}
                />

                {values.map(value => {
                    return (
                        <Picker.Item
                            label={value}
                            value={value}
                        />
                    );
                })}
            </Picker>
        );
    }
}

The resulting this.props.children is:

[
    (child),
    [
        (child),
        (child),
    ],
];

Therefor you can't use this.props.children[2] to get the last item.

The Android version of the Picker does exactly this to get the value of the selected item to pass to the onValueChange prop.

This PR fixes that by normalising the children with React.Children.toArray.

- `this.props.children` is not always a flat array.

ghost · 2016-06-16T15:11:52Z

By analyzing the blame information on this pull request, we identified @mkonicek and @spicyj to be potential reviewers.

ghost · 2016-06-16T15:11:53Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

ghost · 2016-06-16T15:12:20Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

mkonicek · 2016-06-17T22:05:02Z

The example seems reasonable but doing this the conversion in every change event seems unnecessary - the children don't change.

Is it possible to do the conversion just once? Alternatively, could you create a flat list in your application code?

[
  <Picker.Item label="n/a" />
].concat(values.map(value => {
  ...
}))

justim · 2016-06-18T00:06:00Z

The workaround I had so far was indeed to create one list of values and map them to a list of picker items.

Doing the conversion only on mount would not be enough in some cases (my case would be solved, but still), the first item might not always be available in the picker, same goes for any of the values — the children might change.

I'm happy to go either way.

mkonicek · 2016-06-22T05:48:12Z

Doing the conversion only on mount would not be enough in some cases (my case would be solved, but still), the first item might not always be available in the picker, same goes for any of the values — the children might change.

Could you convert the children to an array only when they change?
https://facebook.github.io/react/docs/component-specs.html

I'd argue, however, that converting children to an array automatically could confusing to the reader - I'd prefer the user to explicitly convert whatever data structure they have to an array for readability and clarity.

@spicyj What do you think?

ghost · 2016-07-16T17:01:11Z

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @mkonicek as a potential reviewer. Could you take a look please or cc someone with more context?

sophiebits · 2016-08-18T22:19:45Z

Isn't the fact that it needs to be an array an implementation detail of Picker? This seems okay to me and better than what's currently here.

AzizAK · 2016-08-23T12:33:36Z

@justim i think you can fix it by passing the initial item to the array:

class Example extends Component {

    render() {
        const values = ['1', '2'];
        let pickerItem = []
        pickerItem.push(<Picker.Item label="n/a" value={null} />)
        values.map(value => {
          pickerItem.push(
            <Picker.Item
                label={value}
                value={value}
            />
          )
        })
        return (
            <Picker
                value={this.state.value}
                onValueChange={this.onValueChange.bind(this)}
            >
              {pickerItem}
            </Picker>
        );
    }
}

Please correct me if wrong.

satya164 · 2016-08-23T14:02:17Z

@mkonicek : Could you convert the children to an array only when they change?

This can be easily fixed by using a memoized function to get the current children. But is it really necessary? How slow is the conversion?

hramos · 2016-12-22T22:46:31Z

It's been a while since this PR has seen any activity. Is there any interest in landing this, any additional feedback?

satya164 · 2016-12-23T12:40:53Z

@hramos I think we should land this.

facebook-github-bot · 2017-01-19T21:48:11Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

everyusernameistaken123 · 2017-01-20T09:34:43Z

I am experiencing a related issue.
Can't believe it takes 7months to merge 1 changed line?

satya164 · 2017-01-20T09:40:17Z

@facebook-github-bot shipit

facebook-github-bot · 2017-01-20T21:48:12Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

mkonicek · 2017-02-02T11:53:30Z

Trying again.

@facebook-github-bot shipit

facebook-github-bot · 2017-02-02T12:48:20Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

facebook-github-bot · 2017-03-22T10:27:43Z

@javache has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

satya164 · 2017-03-22T15:43:38Z

cc @lacker

facebook-github-bot · 2017-03-23T11:43:11Z

@javache has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

…a position Summary: When using the following component, `this.props.children` is not a flat array. ``` js class Example extends Component { // ... render() { const values = ['1', '2']; return ( <Picker value={this.state.value} onValueChange={this.onValueChange.bind(this)} > <Picker.Item label="n/a" value={null} /> {values.map(value => { return ( <Picker.Item label={value} value={value} /> ); })} </Picker> ); } } ``` The resulting `this.props.children` is: ``` js [ (child), [ (child), (child), ], ]; ``` Therefor you can't use `this.props.children[2]` to get the last item. The Android version of the [Picker](https://facebook.github.io/react-native/do Closes facebook#8153 Differential Revision: D4753480 Pulled By: javache fbshipit-source-id: deb0264746b39303e66c69c191af0c962db39085

danielfttorres · 2017-05-21T20:54:08Z

@justim it's fix this #12829?

justim · 2017-05-22T12:10:52Z

@danielfeelfine, yes, it should. TheReact.Children.toArray call in this PR handles the nested arrays.

romjear12 · 2017-09-17T18:38:41Z

I used this "hack":

  render() {
    const values = ['1', '2'];

    return (
      <Picker
        value={this.state.value}
        onValueChange={this.onValueChange.bind(this)}
      >
      {
				<Picker
					value={this.state.value}
					onValueChange={this.onValueChange.bind(this)}
				>
				{
					[<Picker.Item
						label="n/a"
						value={null}
					/>].concat(values.map(value => {
							return (
								<Picker.Item
									label={value}
									value={value}
								/>
							)
							})
					) 
				}
				</Picker>
    );
  }

justim · 2017-09-17T21:30:52Z

There is no need for hacks anymore, this PR is merged and the example I gave at the top works now.

Convert children to an array before accessing with a position

bac6ed4

- `this.props.children` is not always a flat array.

ghost added the GH Review: review-needed label Jun 16, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 16, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 12, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 16, 2016

ryankask mentioned this pull request Aug 5, 2016

Error in onvaluechange function in Picker Cannot read property 'props' of undefined #9209

Closed

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 23, 2016

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 23, 2016

ryankask mentioned this pull request Sep 16, 2016

Picker: undefined is not an object (evaluating 'this.props.children[position].props') #9927

Closed

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jan 19, 2017

facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Jan 19, 2017

Merge branch 'master' into picker_android_fix

28d5ad2

facebook-github-bot added Import Started This pull request has been imported. This does not imply the PR has been approved. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Jan 20, 2017

facebook-github-bot added Import Started This pull request has been imported. This does not imply the PR has been approved. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 2, 2017

Merge branch 'master' into picker_android_fix

dce6810

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Mar 15, 2017

Merge branch 'master' into picker_android_fix

a4241b9

facebook-github-bot closed this in 720e195 Mar 23, 2017

danielfttorres mentioned this pull request May 22, 2017

Picker with dynamic options broke on Android #12829

Closed

hramos added the Merged This PR has been merged. label Mar 8, 2019

react-native-bot removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Mar 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Picker (android): Convert children to an array before accessing with a position #8153

Picker (android): Convert children to an array before accessing with a position #8153

justim commented Jun 16, 2016

ghost commented Jun 16, 2016

ghost commented Jun 16, 2016

ghost commented Jun 16, 2016

mkonicek commented Jun 17, 2016

justim commented Jun 18, 2016

mkonicek commented Jun 22, 2016

ghost commented Jul 16, 2016

sophiebits commented Aug 18, 2016

AzizAK commented Aug 23, 2016 •

edited

Loading

satya164 commented Aug 23, 2016

hramos commented Dec 22, 2016

satya164 commented Dec 23, 2016

facebook-github-bot commented Jan 19, 2017

everyusernameistaken123 commented Jan 20, 2017

satya164 commented Jan 20, 2017

facebook-github-bot commented Jan 20, 2017

mkonicek commented Feb 2, 2017

facebook-github-bot commented Feb 2, 2017

facebook-github-bot commented Mar 22, 2017

satya164 commented Mar 22, 2017

facebook-github-bot commented Mar 23, 2017

danielfttorres commented May 21, 2017

justim commented May 22, 2017

romjear12 commented Sep 17, 2017

justim commented Sep 17, 2017

Picker (android): Convert children to an array before accessing with a position #8153

Picker (android): Convert children to an array before accessing with a position #8153

Conversation

justim commented Jun 16, 2016

ghost commented Jun 16, 2016

ghost commented Jun 16, 2016

ghost commented Jun 16, 2016

mkonicek commented Jun 17, 2016

justim commented Jun 18, 2016

mkonicek commented Jun 22, 2016

ghost commented Jul 16, 2016

sophiebits commented Aug 18, 2016

AzizAK commented Aug 23, 2016 • edited Loading

satya164 commented Aug 23, 2016

hramos commented Dec 22, 2016

satya164 commented Dec 23, 2016

facebook-github-bot commented Jan 19, 2017

everyusernameistaken123 commented Jan 20, 2017

satya164 commented Jan 20, 2017

facebook-github-bot commented Jan 20, 2017

mkonicek commented Feb 2, 2017

facebook-github-bot commented Feb 2, 2017

facebook-github-bot commented Mar 22, 2017

satya164 commented Mar 22, 2017

facebook-github-bot commented Mar 23, 2017

danielfttorres commented May 21, 2017

justim commented May 22, 2017

romjear12 commented Sep 17, 2017

justim commented Sep 17, 2017

AzizAK commented Aug 23, 2016 •

edited

Loading