Skip to content

[Android][Fix] Revert Picker item original color#25750

Closed
cabelitos wants to merge 1 commit into
facebook:masterfrom
cabelitos:picker-fix
Closed

[Android][Fix] Revert Picker item original color#25750
cabelitos wants to merge 1 commit into
facebook:masterfrom
cabelitos:picker-fix

Conversation

@cabelitos
Copy link
Copy Markdown
Contributor

@cabelitos cabelitos commented Jul 20, 2019

Summary

Since the Android's Picker implementation uses an ArrayAdapter,
it means that the views that were created may be reused for other items
in order to save memory. With this in mind, if one sets the Picker.Item
prop color for only certain items there might be an state that
some items that does not have the color set will end up appearing
with the wrong color. This happens because, this new item is
reusing a view of an item that had the color prop set.
In order to avoid this problem, once a new view is created
the ReactPickerAdapter will save the original color and
re-apply if the item does not have the color prop.

Changelog

[Android] [Fixed] - Picker.Item displays wrong colors

Test Plan

On android execute the code below. Only the FIRST item should be red.

import React from 'react';
import { StyleSheet, View, Picker } from 'react-native';

const values = new Array(100);

for (let i = 0; i < values.length; i += 1) {
  values[i] = (i * i).toString();
}

const App = () => {
  const [selected, setSelected] = React.useState(0);
  const onValueChange = React.useCallback((_, idx) => {
    setSelected(idx);
  }, []);
  return (
    <View style={styles.container}>
      <Picker onValueChange={onValueChange} selectedValue={values[selected]}>
        {values.map((v, i) => (
          <Picker.Item
            key={v}
            value={v}
            label={v}
            {...(!i ? { color: 'red' } : {})}
          />
        ))}
      </Picker>
    </View>
  );
};

export default App;

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'center',
    paddingHorizontal: 20,
  },
});

Without the patch

You should see various items with the red color (when only the first one should be red)
picker-not-working

With the patch

Only the first item is red.

picker-working

Closes #25456

@facebook-github-bot 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 Jul 20, 2019
Copy link
Copy Markdown
Contributor

@mdvacca mdvacca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are touching this file, can you return textView instead?

Copy link
Copy Markdown
Contributor

@mdvacca mdvacca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @cabelitos, thanks for fixing this!
It looks good to me, if you have 5 min to make that change it would be great. Otherwise I will just land this as it is!

@cabelitos
Copy link
Copy Markdown
Contributor Author

Thank you for your review @mdvacca. I'm doing the change right now!

Since the Android's Picker implementation uses an ArrayAdapter,
it means that the views that were created may be reused for other items
in order to save memory. With this in mind, if one sets the Picker.Item
prop color for only certain items there might be an state that
some items that does not have the color set will end up appearing
with the wrong color. This happens becase, this new item is
reusing a view of an item that had the color prop set.
In order to avoid this problem, once a new view is created
the ReactPickerAdapter will save the original color and
re-apply if the item does not have the color prop.
@cabelitos
Copy link
Copy Markdown
Contributor Author

@mdvacca did the change, please let me know what you think.
And once again thank you and your colleagues for the support.

@mdvacca
Copy link
Copy Markdown
Contributor

mdvacca commented Jul 22, 2019

Looks great, thanks!

I'm landing the PR

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdvacca is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @cabelitos in 5b953e5.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: Android Android applications.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

React Native Picker shows ghost picked value

4 participants