-
Notifications
You must be signed in to change notification settings - Fork 85
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
[WNMGDS-2714] Fix Dialog
and HelpDrawer
analytics firing too early
#3055
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.
seeing weird behavior for onEnter
with Dialog (discussed in office hours)
@@ -52,9 +62,31 @@ describe('HelpDrawer', () => { | |||
jest.resetAllMocks(); | |||
}); | |||
|
|||
it('sends analytics event tracking on open help drawer', () => { | |||
renderHelpDrawer(); | |||
it("does not send analytics event when dialog isn't open", () => { |
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.
Does calling the tests "dialog" matter if it's underlying element is <dialog>
🤔
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.
doh!
I can find no evidence of people using it, and it has the same defect as #3055
#3055) * Fix `Dialog` firing open and close events at wrong time * Separate that logic into a hook so it can be shared * Apply the fix to `HelpDrawer` too * Update snapshots again after rebasing * Fix analytics in help drawer story * Fix copypasta in test names * Oh, snapshots have to be updated when the names change
Summary
Fixes
Dialog
andHelpDrawer
analytics open event firing before it's actually opened. When we changed theNativeDialog
API to use anisOpen
prop to control its open state, it meant that rendering aNativeDialog
was no longer equivalent to opening a dialog. Our analytics events forDialog
andHelpDrawer
open and close were based on mounting and unmounting the components, so when one of those comonents rendered, it would fire the open event even ifisOpen
wasfalse
.How to test
Checklist
[WNMGDS-####] Title
or [NO-TICKET] if this is unticketed work.Type
(only one) label for this PR, if it is a breaking change, label should only beType: Breaking
Impacts
, multiple can be selected.If this is a change to code:
yarn test:unit:update
) and browser-test snapshots (yarn test:browser:update
)