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

Add Icon prop to component #105

Closed

Conversation

luizParreira
Copy link

  • Adding the possibility of the user passing a custom Icon prop to be
    rendered instead of the default one
  • Added README.md instructions
  • Add tests for the new behaviour

Closes #104

Copy link
Collaborator

@lfkwtz lfkwtz left a comment

Choose a reason for hiding this comment

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

could you fix the merge conflicts -- renderIcon works with both ios/android due to the new flag added in 5.1, so if you could update the testIDs accordingly too I would appreciate it--- thanks

src/index.js Outdated
@@ -416,6 +423,7 @@ export default class RNPickerSelect extends PureComponent {
>
{this.renderPickerItems()}
</Picker>
{Icon && this.renderIcon()}
Copy link
Collaborator

Choose a reason for hiding this comment

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

not a fan of this syntax personally -- would prefer:

{Icon ? this.renderIcon() : null}

@luizParreira luizParreira force-pushed the feature/add-custom-icon branch 2 times, most recently from e397da3 to ae333fb Compare November 14, 2018 19:03
- Adding the possibility of the user passing a custom `Icon` prop to be
rendered instead of the default one
- Added README.md instructions
- Add tests for the new behaviour
- Change testID keys to not mindd about platforms anymore
- Refactored renderAndroid because of `Cognitive Complexity` issue on
code climate
@luizParreira
Copy link
Author

@lfkwtz the commit ce056cc has made the changes requested 😃

@lfkwtz
Copy link
Collaborator

lfkwtz commented Nov 18, 2018

Can you share some sample code of this working as intended on ios and android? Ideally on snack.expo.io

@balthazar
Copy link

Any plan to get this merged? Since @luizParreira already did the testing, adding one picker using the new prop in the example file should be enough no?

@lfkwtz
Copy link
Collaborator

lfkwtz commented Dec 1, 2018

I tested it myself locally and it wasn’t working as I imagine it should - which is why I’m asking for working examples

@balthazar
Copy link

I see, makes sense 👍

@lfkwtz
Copy link
Collaborator

lfkwtz commented Dec 19, 2018

@luizParreira

@lfkwtz
Copy link
Collaborator

lfkwtz commented Jan 3, 2019

Going to close this out as I haven't heard from you in a couple months. We can re-open later.

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

Successfully merging this pull request may close these issues.

None yet

3 participants