- 
                Notifications
    You must be signed in to change notification settings 
- Fork 276
          feat: User Event type()
          #1396
        
          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
              
            Conversation
65b98db    to
    803d8ed      
    Compare
  
    221d037    to
    0d089c2      
    Compare
  
    8a40e8e    to
    acf58dc      
    Compare
  
    1483610    to
    4eaecde      
    Compare
  
    e0ce685    to
    4cd2d35      
    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.
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.
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.setupwaitutilitytype()API:TextInputsperforms input transformationTextInputelementsExclusions:
targetandeventCountevent props (just pass0there)Test plan
Added tests covering 100% specs