-
Notifications
You must be signed in to change notification settings - Fork 72
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
Dispatch Fides.js lifecycle events on window (FidesInitialized, FidesUpdated) and cross-publish to Fides.gtm() integration #3454
Conversation
This should be good to go. Just want to triple-check the event publishing to GTM. I'd also like to add the |
Passing run #2436 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
Did some more research on GTM this morning and I'm quite confident that I can safely add Still want to do some manual testing before merging this, but going to adjust that now and open up for review... |
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.
Few comments to help explain what's going on
@@ -82,6 +82,8 @@ type MetaOptions = { | |||
/** | |||
* Call Fides.meta to configure Meta Pixel tracking. | |||
* | |||
* DEFER: Update this integration to support async Fides events |
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.
Add follow-up issue
@@ -33,6 +33,8 @@ const applyOptions = (options: ShopifyOptions) => { | |||
* Call Fides.shopify to configure Shopify customer privacy. Currently the only consent option | |||
* Shopify allows to be configured is user tracking. | |||
* | |||
* DEFER: Update this integration to support async Fides events |
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.
Add follow-up issue
/** | ||
* Defines the properties available on event.detail. As of now, these are an | ||
* explicit subset of properties from the Fides cookie | ||
* TODO: add identity and meta? |
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.
Decide on this - I think it makes sense to include these in the event, I could see a listener wanting a bit of information about the identity when processing the preferences, etc.
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.
Will facebook / shopify need identity or other metadata?
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.
Nothing needs it yet, but I'm wondering what a developer might need when integrating consent into their app via these events...
// Update the cookie object | ||
// eslint-disable-next-line no-param-reassign | ||
cookie.consent = consentCookieKey; |
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 mutates the provided cookie
param, which might be a bit surprising... but I think it makes sense to do this as we don't store that state anywhere else super declaratively. This is especially true since we call saveFidesCookie
below which persists the updated state to the browser - it'd be surprising to me to find that the cookie
object was actually more stale than the version saved to browser since we weren't updating that object before!
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 is fine. It's the same pattern we use in the init
method of fides.ts
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.
Few comments to help explain what's going on
// Pick a subset of the Fides cookie properties to provide as event detail | ||
const { consent }: FidesEventDetail = cookie; | ||
const detail: FidesEventDetail = { consent }; | ||
const event = new CustomEvent(type, { detail }); |
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.
Ah this is cool, learned about CustomEvent
native obj today
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.
Looks good so far.
Main thing I'd like to see updated is in fides-js-components-demo.html
and fides-js-demo.html
, we are currently using a timer to check when fides is initialized. We can now replace this with the addEventListener
you've added. Feel free to create a new ticket, and I can take care of it, either way.
Blergh - I need to edit these tests to work still. |
Closes #3350
Closes #3348
Code Changes
window
whenFides.init()
completes successfullywindow
whenever preferences are updatedFides.gtm()
Google Tag Manager integration to listen for lifecycle events and publish to GTM whenever consent preferences are initialized/updatedSteps to Confirm
Pre-Merge Checklist
CHANGELOG.md
Description Of Changes
This exposes two new custom events on the
window
object to allow a user to listen for Fides.js lifecycle events and integrate consent state changes into their own custom Javascript handling. This is needed now that we allow preferences to change at runtime via the banner/modal components!For example, you might integrate Fides.js into your website like:
This PR also updates the
Fides.gtm()
integration to listen to the same lifecycle events and cross-publish them to GTM. This ensures GTM stays up to date if consent preferences are updated by the banner/modal.