-
Notifications
You must be signed in to change notification settings - Fork 273
docs: add migration to v7 guide #456
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
Conversation
30ad0c5
to
5cc1007
Compare
We can merge this guide before the actual migration, because it won't show up in the guides just yet. It will be hidden under "/react-native-testing-library/docs/migration-v7" link for now. EDIT: nevermind, I've created 7.x branch so we can merge anytime anyway. |
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.
Solid 💯 I'm thrilled how simple this will be to migrate. Thank you for all your work @thymikee!
bbb8b18
to
116ef86
Compare
116ef86
to
3e9bfbc
Compare
3d59e3f
to
7b564de
Compare
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.
Overally LGTM. Some questions and suggestions.
|
||
## No special handling for `disabled` prop | ||
|
||
The `disabled` prop on "Touchable\*" components is treated in the same manner as any other prop. We realize that with our library you can press "touchable" components even though they're in "disabled" state, however this is something that we strongly believe should be fixed upstream, in React Native core. |
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.
From what I can read in RNTL code it seems that the logic is to find prop with given name (e.g. onPress
for press
event) and just run it. I'm not sure how this should be fixed by RN, so that we could use it in RNTL, as we are executing the prop function without delegating event logic to RN. One option for RN would be to automatically wrap onPress
assignment which would add if(!disabled)
check, but this will actually assign a different function there.
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.
We could try adding onStartShouldSetResponder
check support, which should solve this particular problem. Likely not thorough enough to improve whole event firing system, but maybe it's a good start.
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.
Went ahead and merged the guide, we can still amend it when we decide to act on this topic
Co-authored-by: Maciej Jastrzebski <[email protected]>
Co-authored-by: Maciej Jastrzebski <[email protected]>
Co-authored-by: Maciej Jastrzebski <[email protected]>
Co-authored-by: Maciej Jastrzebski <[email protected]>
* docs: add migration to v7 guide * adjust note about ByTitle because there's no host Button component * add fire event section * mention NativeTestInstance and container * Update website/docs/MigrationV7.md Co-authored-by: Maciej Jastrzebski <[email protected]> * Update website/docs/MigrationV7.md Co-authored-by: Maciej Jastrzebski <[email protected]> * Update website/docs/MigrationV7.md Co-authored-by: Maciej Jastrzebski <[email protected]> * Update website/docs/MigrationV7.md Co-authored-by: Maciej Jastrzebski <[email protected]> Co-authored-by: Maciej Jastrzebski <[email protected]>
* docs: add migration to v7 guide * adjust note about ByTitle because there's no host Button component * add fire event section * mention NativeTestInstance and container * Update website/docs/MigrationV7.md Co-authored-by: Maciej Jastrzebski <[email protected]> * Update website/docs/MigrationV7.md Co-authored-by: Maciej Jastrzebski <[email protected]> * Update website/docs/MigrationV7.md Co-authored-by: Maciej Jastrzebski <[email protected]> * Update website/docs/MigrationV7.md Co-authored-by: Maciej Jastrzebski <[email protected]> Co-authored-by: Maciej Jastrzebski <[email protected]>
Summary
Closes #442
Test plan