-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Create custom TextInput #3414
Create custom TextInput #3414
Conversation
Un-assigning myself as this isn't ready for review yet. Feel free to assign pullerbear again once ready for reviews! |
@shawnborton Could you take a look at this PR? |
Can do, will take a pass at this today! |
For select/picker menus, can we make it so that you can tap anywhere on the picker to launch the menu? Currently it looks like it only works if you tap on the value text, but if you tap the label or anything else, it doesn't launch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested again on the devices that were giving us trouble before, and everything seems great! Time to merge :D
@roryabraham back to you (since you requested changes recently) - also thanks so much for the help reviewing this beast of a PR! |
@roryabraham requesting a review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we took this change a bit too far. Can we undo the last commit, and then just make the simple change of getting rid of styles.picker.inputAndroid
and styles.picker.inputIOS
, and replacing them with styles.picker.inputNative
.
inputAndroid: styles.expensiPicker(disabled).inputNative, | ||
}); | ||
|
||
pickerStyles.propTypes = propTypes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a style, not a react component. So it doesn't make sense to include propTypes
, defaultProps
, or displayName
.
Approved, we can merge as soon as iOS E2E tests pass. |
Thank you. I was waiting for e2e tests to request a review again) |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @roryabraham in version: 1.0.83-2 🚀
|
Left a note on the issue linked above but also tagging here, this PR is causing a deploy blocker: #4597 (comment). @kakajann please have a look |
This has been deployed to production and is now subject to a 7-day regression period. |
I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check. We've also updated the item in the checklist with this PR to avoid this issue in the future. |
Details
Fixed Issues
Fixes #3108
Tests
Make sure form inputs match specs & screenshots below
QA Steps
Same as above
Tested On
Specific Specs
Screenshots
Web
web.mov
Mobile Web
web.mobile.mov
Desktop
desktop.app.mov
iOS
ios.mov