-
Notifications
You must be signed in to change notification settings - Fork 273
Generate event object for fireEvent #1171
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
Changes from 8 commits
5ab3064
72ef1aa
a1f681c
e58cff2
a2afde3
60ffcca
063c4f5
4b908ba
454926a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,10 +115,17 @@ const invokeEvent = ( | |
return; | ||
} | ||
|
||
// this is just a placeholder and needs to be a more realistic object | ||
const generatedEventObject = { someKey: 'value' }; | ||
|
||
let defaultCallbackValues = | ||
eventName === 'changeText' ? [] : [generatedEventObject]; | ||
const handlerCallbackValues = data.length > 0 ? data : defaultCallbackValues; | ||
|
||
let returnValue; | ||
|
||
act(() => { | ||
returnValue = handler(...data); | ||
returnValue = handler(...handlerCallbackValues); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally we would like to follow DTL's
In DTL they have |
||
|
||
return returnValue; | ||
|
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.
AFAIK events by convention take a single argument of
Event
type or subtype. We also haveonChangeText
which gets a singlestring
value, but that seems to be simplified version ofonChange
. I would assume that we can focus only on handling single argument. That should simplify the typing, while we can always refactor if there would be any such 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.
Thanks for this info! BTW I noticed that I didn't clearly link to our additional questions from here. Here they are: #714 (comment)