35102/Banner Web Component #284
Conversation
pdavies88
added
minor
pdavies88
requested review from
bkjohnson,
andresriveratoro,
mendacitees and
k80bowman
…-veterans-affairs/component-library into 35102/Banner-Web-Component
|
|
||
| // This querySelector finds the Mock Element but it is not a E2EEElement | ||
| // E2EEElements seem to be reserved to only the hosts shadowroot elements like va-alert | ||
| const button = await vaAlert.shadowRoot.querySelector('button'); |
There was a problem hiding this comment.
How about const button = await page.find('pierce/button') ?
There was a problem hiding this comment.
I meant to comment not request changes. Sorry!
There was a problem hiding this comment.
Thank you this was exactly what I was looking for to get the test working!
There was a problem hiding this comment.
Is that pierce/button a puppeteer thing or a stencil thing?
There was a problem hiding this comment.
It's from pupeeter. We're also using this in tests for va-additional-info and va-select.
|
|
||
| expect(Array.from(vaBanner.shadowRoot.childNodes)).toHaveLength(0); | ||
|
|
||
| await page.evaluate(() => { |
There was a problem hiding this comment.
Testing a page refresh to ensure that the banner is still dismissed
| // Track the dismiss event in Google Analytics | ||
| if (!this.disableAnalytics) { | ||
| const detail = { | ||
| componentName: 'Banner', |
There was a problem hiding this comment.
I know some of the early web components used their React component names for this, but going forward we should probably use the web component name. Correct me if I'm wrong @bsmartin-ep
There was a problem hiding this comment.
You are correct @bkjohnson. We have been matching the web component name.
There was a problem hiding this comment.
@bsmartin-ep thank you I will update the component name. However a couple of questions:
- What would you like the details of the Analytics event to be for dismiss? Right now it is clickLabel: "Dismiss Banner", headline: string, showClose: boolean, windowSession: boolean, type: string
- We have a va-alert component nested inside our va-banner component. Therefore the analytics event for the va-alert that is enabled for when a link is clicked will fire inside of the va-banner component. Do we need a second analytics event that would be exclusive to the va-banner for when a link is clicked?
There was a problem hiding this comment.
Apologies - missed these questions earlier.
- This is event
int-alert-box-close, correct? We're only looking foralert-box-headlinein the dataLayer. Which got me thinking this setup file isn't working correctly. We just kebab case the component name and prepend it to the dataLayer key. Which won't work for us here, since we'll getva-alert-headline. Want me to send in a PR allowing us to set an analytics prefix in that setup file? - No - unless there are new event parameters we need to add, I think deferring to the
va-alertevent sounds good to me.
There was a problem hiding this comment.
@bsmartin-ep thanks for the info yes the original event is int-alert-box-close.
So correct me if I am wrong but I believe the event would become va-banner-alert-box-headline
I would need to add in:
'va-banner': [{ action: 'click', event: 'int-alert-box-close' }],
and
'va-alert': [{ action: 'linkClick', event: 'nav-warning-alert-box-content-link-click' }],
since they don't exist there yet
There was a problem hiding this comment.
So correct me if I am wrong but I believe the event would become va-banner-alert-box-headline
Unfortunately I don't think this will work for us. Our tagging system gives us some flexibility with regex-ing event names but not parameters. We would have to create a mess of fall-through default values in GTM.
I think our best bet is to move away from the kebab-casing of the component name and add in the GA event parameter prefix we want to the lookup table:
'va-alert': [
{ action: 'linkClick', parameter_prefix: 'alert-box', event: 'nav-warning-alert-box-content-link-click' }
],...or maybe slightly restructuring the array into an object...
'va-alert': {
parameter_prefix: 'alert-box',
events: [{ action: 'linkClick', , event: 'nav-warning-alert-box-content-link-click' }]
},Happy to send in a PR. Or fix this in a separate concurrent issue (since your changes didn't cause this preexisting issue).
| details: { | ||
| clickLabel: 'Dismiss Banner', | ||
| headline: this.headline, | ||
| showClose: this.showClose, | ||
| windowSession: this.windowSession, | ||
| type: this.type, | ||
| }, |
There was a problem hiding this comment.
We may want to ask @bsmartin-ep what kinds of data we should capture in the analytics event for this since <Banner> just had a simple recordEvent().
| private handleAlertBodyClick(e: MouseEvent): void { | ||
| if (!this.disableAnalytics) { | ||
| const target = e.target as HTMLElement; | ||
| // If it's a link being clicked, dispatch an analytics event | ||
| if (target?.tagName === 'A') { | ||
| const detail = { | ||
| componentName: 'Banner', | ||
| action: 'linkClick', | ||
| details: { | ||
| clickLabel: target.innerText, | ||
| headline: this.headline, | ||
| showClose: this.showClose, | ||
| type: this.type, | ||
| }, | ||
| }; | ||
| this.componentLibraryAnalytics.emit(detail); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks like a simplified version of what's already happening in the <va-alert> with the componentName changed. Do we also need to do this for the banner?
If we do need to do it, maybe we could have a handler for the component-library-analytics event which stops the event from bubbling with .stopPropogation() then fires out a new analytics event. This event could have the same data except componentName gets changed to va-banner.
There was a problem hiding this comment.
@bkjohnson good point on the double analytics event firing. I had added it in because in the React Component version of the Banner we had a recordEvent() being fired when an A tag was clicked in the banner here:
But since we are already tracking that event in the Alert Web component I guess we probably don't need it in the banner. Do you think we should have different events for the A tag clicking in the different components or is this a larger question that we need to discuss with the Analytics team?
| // Derive dismissed banners from storage. | ||
| const dismissedBannersString = storageType.getItem(DISMISSED_BANNERS_KEY); | ||
| this.dismissedBanners = dismissedBannersString | ||
| ? JSON.parse(dismissedBannersString) | ||
| : []; |
There was a problem hiding this comment.
Do we need to do this if showClose is false?
There was a problem hiding this comment.
I updated it to wrap it in a conditional so that it will only fire if showClose is true
| status={this.type} | ||
| > | ||
| <h3 slot="headline">{this.headline}</h3> | ||
| <slot></slot> |
There was a problem hiding this comment.
I'm thankful that once this migration happens we won't be using dangerouslySetInnerHTML 😅
packages/web-components/src/components/va-banner/test/va-banner.e2e.ts
Outdated
Show resolved
Hide resolved
| it('fires an analytics event when a link is clicked', async () => { | ||
| const page = await newE2EPage(); | ||
| await page.setContent( | ||
| '<va-banner headline="This is a test">Test Content<a href="#">Test Link</a></va-banner>', | ||
| ); | ||
|
|
||
| const analyticsSpy = await page.spyOnEvent('component-library-analytics'); | ||
|
|
||
| const link = await page.find('va-banner a'); | ||
| await link.click(); | ||
|
|
||
| expect(analyticsSpy).toHaveReceivedEventDetail({ | ||
| componentName: 'Banner', | ||
| action: 'linkClick', | ||
| details: { | ||
| clickLabel: 'Test Link', | ||
| headline: 'This is a test', | ||
| showClose: false, | ||
| type: 'info', | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Would component-library-analytics be firing twice - once from the alert and once from the banner? If that's the behavior we prefer, let's also test for both events firing.
…-veterans-affairs/component-library into 35102/Banner-Web-Component
Per conversation with Brian on Analytics Team
|
|
||
| return ( | ||
| <Host | ||
| data-e2e-id="emergency-banner" |
There was a problem hiding this comment.
This might not be necessary anymore. It may have been in <Banner> originally for some nightwatch tests, but I'm not seeing emergency-banner used anywhere in vets-website cypress tests.
There was a problem hiding this comment.
Excellent going to remove
…-veterans-affairs/component-library into 35102/Banner-Web-Component
Description
Closes department-of-veterans-affairs/va.gov-team#35102
Testing done
Screenshots
Acceptance criteria
Definition of done