-
Notifications
You must be signed in to change notification settings - Fork 274
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: User Event type()
#1396
feat: User Event type()
#1396
Conversation
65b98db
to
803d8ed
Compare
221d037
to
0d089c2
Compare
8a40e8e
to
acf58dc
Compare
1483610
to
4eaecde
Compare
e0ce685
to
4cd2d35
Compare
</Text> | ||
); | ||
|
||
dispatchOwnHostEvent(screen.getByTestId('text'), 'press', TOUCH_EVENT); |
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 don't find this naming very clear, I understood what it meant but it needed some reflexion. Not sure I could come up with a better one though this one is tricky. How about dispatchHostEventWithoutBubbling?
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.
Yeah naming here is difficult.
As far as I understand the React event processing, capture and bubbling phases, the looking up just for the first ancestor is not actually bubbling, as bubbling would be invoking given event for all ancestors listening for it. So using bubbling might be confusing in this case.
function getEnabledEventHandler( | ||
element: ReactTestInstance, | ||
eventName: string | ||
): EventHandler | null { | ||
const touchResponder = isTouchResponder(element) ? element : undefined; | ||
|
||
const handler = getEventHandler(element, eventName); | ||
if (handler && isEventEnabled(element, eventName, touchResponder)) { |
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 don't think we shoud use the function isEventEnabled for userEvent. It is used for any kind of events and supports every eventName, whereas here we could have a pretty straightforward check on the editable prop. We know we want to type something and not do something else and that allows better handling for specific events, which fireEvent lacks because it's generic and I think we should use that for userEvent
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 have to check experimentally if parent pointerEvents
could block TextInput
focus, if not then we could just inspect the editable
prop.
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.
Oh I hadn't thought about that, I'll probably need to check for pointer events as well for Text and TextInput for user.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.
So on iOS: pointerEvents="none" on either View wrapping TextInput or TextInput itself prevents TextInput from gaining focus
On Android: pointerEvents="none" on either View wrapping TextInput or prevents TextInput from gaining focus, but on TextInput itself is not preventing focus 🤦♂️
2999933
to
227a77b
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1396 +/- ##
==========================================
+ Coverage 97.02% 97.53% +0.51%
==========================================
Files 68 72 +4
Lines 3863 4225 +362
Branches 568 624 +56
==========================================
+ Hits 3748 4121 +373
+ Misses 115 104 -11
☔ View full report in Codecov by Sentry. |
This looks really good! I have one question left though: a lot of code that's in |
Good point, in the specific case of TextInput we can do single check on TextInput if it's enabled. I wonder if we could do something similar for more general case, e.g. When doing |
chore: restore tests chore: more tests chore: improve coverage chore: moar tests docs: improve the docs chore: fix lint refactor: code review changes chore: fix lint chore: tweak return type refactor: flush microtasks after event refactor: re-organize waits refactor: improve dispatch event functions refactor: remove flushMicroTasks calls
3154b5e
to
5a39b8e
Compare
Rebased after merging |
@pierrezimmermannbam could you take a look at this PR. I've address the remaining comment about making TextInput enable check separately from dispatching the events. I've also greatly simplified the Finally, I've tweaked some of the |
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 looks very good! I just caught some typos in the docs but other than that this is more than what I had expected for a v1 of userEvent, i look forward to trying it!
🎉 This PR has been released in v12.2.0 🚀 |
Summary
Minimal User Event
type()
implementation including initial User Eventsetup()
API mimicking RTL's one.Scope:
userEvent.setup
wait
utilitytype()
API:TextInputs
performs input transformationTextInput
elementsExclusions:
target
andeventCount
event props (just pass0
there)Test plan
Added tests covering 100% specs