-
Notifications
You must be signed in to change notification settings - Fork 693
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
new driver for hint testing #2898
base: master
Are you sure you want to change the base?
Conversation
const hintTouchableDriver = usePressableDriver<TouchableOpacityProps>(useComponentDriver({ | ||
renderTree: props.renderTree, | ||
testID: `${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.
Isn't this just the Hint
itself?
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.
yes it is, do you think that the naming isn't good ?
It's the Touchable
part of the component with out the View
wrapper
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.
Yes, I think it should be exported as press
and probably something like pressOnBackground: overlayDriver.press
for the overlay (same as in Modal
)
@adids1221 I'd also remove the comment from the change log 🙏 |
Also, nice work on the mock, I'm sure that was not easy 👍 |
@M-i-k-e-l fix most of the comments, about the |
|
||
const WhiteHint = <HintTestComponent showHint color={Colors.white}/>; | ||
const WhiteHintDriver = new HintDriver({component: WhiteHint, testID: 'Hint'}); | ||
describe('Test Hint modal visibility:', () => { |
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.
You want to test the Hint's visibility not the Modal's
return hintPressableDriver.press(); | ||
}; | ||
|
||
const hintPressOnBackground = () => { |
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 should probably be pressOnBackground
const getModal = () => { | ||
return modalDriver; | ||
}; |
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 this is an internal implementation detail, you probably want to externalize isVisible
instead
return overlayDriver; | ||
}; | ||
|
||
const hintPress = () => { |
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 should be press
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.
Ping
const getHintPressable = () => { | ||
return hintPressableDriver; | ||
}; |
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.
Please remove unused methods
@M-i-k-e-l didn't finished yet, having some issues with the |
@M-i-k-e-l as we spoke we can't use |
Hi @adids1221, |
const hintPressableDriver = usePressableDriver(useComponentDriver({ | ||
renderTree: props.renderTree, | ||
testID: `${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.
This is simply driver
, it is a duplicate and should be removed
return overlayDriver; | ||
}; | ||
|
||
const hintPress = () => { |
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.
Ping
const getOnBackgroundPressTouchable = () => { | ||
return overlayDriver; | ||
}; |
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 not the convention I was talking about; if you're using getOnBackgroundPressTouchable().exists()
the you should export something like isBackgroundExists()
and not the whole driver.
return overlayDriver; | ||
}; | ||
|
||
const getIsModalVisible = () => { |
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.
IMO there should be an isVisible
for both modal
and overlay
Can't really remember the reason for your previous comment about modal's visibility, lets discuss it if it's still relevant.
Description
Hint - create new driver and refactor tests to it
Changelog
Additional info