-
Notifications
You must be signed in to change notification settings - Fork 2
feat(native): Add toBeVisible #145
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
base: main
Are you sure you want to change the base?
Conversation
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! I left a few suggestion, let me know if you have any question 🙂
35e5052
to
82fa78b
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.
LGTM!
4cfe69c
to
dcded59
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.
Looking great! I left a couple of comments to help make the code easier to understand. Let me know what you think!
@@ -42,7 +42,7 @@ export class ElementAssertion extends Assertion<ReactTestInstance> { | |||
} | |||
|
|||
/** | |||
* Check if the component is enabled. | |||
* Check if the component is enabled and has not been disabled by an ancestor. |
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.
👍🏼
}); | ||
|
||
return this.execute({ | ||
assertWhen: this.isElementVisible(this.actual) && !this.isAncestorNotVisible(this.actual), |
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' reading the second condition like "...and not is ancestor not visible" which is weird 😅 Would it make more sense to invert the logic of the function and change it's name to isAncestorVisible(..)
?
private isElementVisible(element: ReactTestInstance): boolean { | ||
const { type } = element; | ||
const elementType = type.toString(); | ||
if (elementType === "Modal" && !element?.props?.visible === true) { |
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.
!element?.props?.visible === true
This one is also very hard to read. So we first negate the value of an optional prop and the compare it to true? 😖 Is there a better way to write this condition?
Co-authored-by: Jose Luis Leon <[email protected]>
This PR adds the
toBeVisible
matcher for React Native