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

v5 #91

Merged
merged 8 commits into from
Oct 19, 2018
Merged

v5 #91

merged 8 commits into from
Oct 19, 2018

Conversation

lfkwtz
Copy link
Collaborator

@lfkwtz lfkwtz commented Oct 12, 2018

closes #90
closes #78
closes #93

Copy link
Contributor

@robdsoule robdsoule left a comment

Choose a reason for hiding this comment

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

Now that there is some usage and this is really the first breaking version bump I believe this is the time to add a CHANGELOG.md file with at least a quick note on any breaking changes.

src/index.js Outdated
@@ -317,23 +338,27 @@ export default class RNPickerSelect extends PureComponent {
{this.renderTextInputOrChildren()}
</TouchableWithoutFeedback>
<Modal
testID="RNPickerSelectModal"
{...modalProps}
Copy link
Contributor

Choose a reason for hiding this comment

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

Any thoughts on having this destructure happen after our defined modal props?

Having the destructure happen after is more flexible for people using the component, but I could see concerns on it causing unnecessary issues being reported.

return {
// TODO: remove style.placeholderColor option in v5 release
color: this.props.style.placeholderColor || this.props.placeholderTextColor,
color: placeholderTextColor,
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

setTimeout(() => {
this.props.onUpArrow();
});
setTimeout(onUpArrow);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good time to add a comment why setTimeout is on these two arrow methods imo

Copy link
Contributor

@robdsoule robdsoule left a comment

Choose a reason for hiding this comment

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

gtg

@lfkwtz lfkwtz merged commit b5c4cc8 into master Oct 19, 2018
@lfkwtz lfkwtz deleted the placeholder-updates branch October 19, 2018 21:35
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.

Have props for label style
2 participants