-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat(native): Add toBeDisabled #140
base: main
Are you sure you want to change the base?
Conversation
c61ba82
to
190ce45
Compare
4c514fe
to
2259962
Compare
2259962
to
72efbf8
Compare
5817dc1
to
1e2a9c3
Compare
1e2a9c3
to
5fbe42a
Compare
Co-Authored-By: Suany Chalan <79164262+suany0805@users.noreply.github.com>
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.
Looks good so far! There's a few things to address but I think we're on the right path in general. Let me know if you have any questions 🙂
return "null"; | ||
} | ||
|
||
return `<${this.actual.type.toString()} testID="${this.actual.props.testID}"... />`; |
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.
Are we sure about this? Not all elements will have a testID
. Also, using the testID should be our last resource when using tools like testing-library.
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're missing tests for .not.toBeDisabled()
and not.ToBeEnabled()
. The messaging is different, so having some unit tests is good. I'd test them together with the not inverted test cases to make things simpler, check the core
package for examples 🙂
…of main assertive path
0022d85
to
898721a
Compare
No description provided.