-
Notifications
You must be signed in to change notification settings - Fork 785
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(mock-doc): dispatch blur and focus events #3449
feat(mock-doc): dispatch blur and focus events #3449
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.
just a few questions and comments after reading through
src/mock-doc/event.ts
Outdated
export class MockFocusEvent extends MockUIEvent { | ||
relatedTarget: EventTarget | null = null; | ||
|
||
constructor(type: string, focusEventInitDic?: FocusEventInit) { |
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.
probably not crucial, but could we type type
here as "blur" | "focus"
?
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.
Great catch thank you!
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 just waiting on this change, and then it looks good!
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 sorry I missed that one! Just committed
src/mock-doc/test/event.spec.ts
Outdated
}).toThrow(); | ||
}); | ||
|
||
it('FocusEvent(type)', () => { |
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.
just for completion's sake, what would you think about parameterizing this test across "blur", "focus"
? something like this perhaps:
it('FocusEvent(type)', () => { | |
it.each(["blur", "focus"])('creates a %s-type MockFocusEvent', (focusType) => { |
const eventInitDict = { | ||
bubbles: true, | ||
composed: true, | ||
relatedTarget: null as EventTarget | null, |
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.
might be good to have a test where we pass a legit value through for this field, what do you think?
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'm not sure what you mean by legit value?
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.
sorry should have been more explicit! just something that matches the EventTarget
interface — not a super essential test though!
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 checked into this and it doesn't look like any of the Mock
elements (MockInputElement
, etc) inherit from EventTarget
currently.
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.
ok no worries, let's leave it as-is then
Co-authored-by: Alice <alice.octavia.pote@gmail.com>
src/mock-doc/event.ts
Outdated
export class MockFocusEvent extends MockUIEvent { | ||
relatedTarget: EventTarget | null = null; | ||
|
||
constructor(type: string, focusEventInitDic?: FocusEventInit) { |
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 just waiting on this change, and then it looks good!
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!
Going to merge this as an admin - "Tech Debt Burndown / Download error files and report (pull_request)" is failing (we'll fix this next sprint) for external contributors, but the report itself is being generated/reporting no new unused exports/strict null check violations |
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build
) was run locally and any changes were pushednpm test
) were run locally and passednpm run test.karma.prod
) were run locally and passednpm run prettier
) was run locally and passedPull request type
For mock-doc,
blur()
andfocus()
methods will now dispatch an event. CreatedMockFocusEvent
which extends a newMockUIEvent
, which extends the existingMockEvent
.Please check the type of change your PR introduces:
What is the current behavior?
The blur and focus methods are empty.
What is the new behavior?
The methods now dispatch an event (similar to how
click()
works) via theMockFocusEvent
class with atype
property of'blur'
or'focus'
Does this introduce a breaking change?
Testing
Added tests for verify new
MockFocusEvent
instantiationOther information