-
Notifications
You must be signed in to change notification settings - Fork 926
Add function setConsent() to set end user consent state for web apps in Firebase Analytics #6376
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
Conversation
🦋 Changeset detectedLatest commit: 0d0bd61 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (347,240 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
Adding API docs into local branch
/* eslint-enable camelcase */ | ||
|
||
/** Whether a particular consent type has been granted or denied. */ |
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.
You can ask @egilmorez for a more definitive opinion but I guess maybe "should be" instead of "has been" since in this context we can only set it, not read the current state. Also needs a @public
tag.
@@ -288,4 +288,14 @@ export interface EventParams { | |||
page_path?: string; | |||
[key: string]: unknown; | |||
} | |||
|
|||
/** Maps the applicable end user consent state. */ |
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.
Suggest: "Consent status settings for each consent type." or similar, @egilmorez may have suggestions. Also add @public
tag.
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.
Doc strings LGTM, thanks!
ad_storage?: ConsentStatusString; | ||
// Enables storage, such as cookies, related to analytics (for example, visit duration) | ||
analytics_storage?: ConsentStatusString; | ||
// Enables storage that supports the functionality of the website or app such as language settings |
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.
Looking at the generated analytics.api.md, it looks like api-documenter still thinks these are undocumented. I think api-documenter may need comments to be in the /** */ format.
packages/analytics/src/api.ts
Outdated
* Use the {@link ConsentSettings} to specify individual consent type values. By default consent | ||
* types are set to "granted". | ||
* @public | ||
* @param consentSettings Maps the applicable end user consent state for gtag.js. |
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.
Oops, I just ran build
on this branch and noticed warnings from api-documenter that consentSettings
needs to be followed by a hyphen here, and also customParams on line 236, which I think was from a previous PR.
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, just address the param tsdoc comment before merging.
Sets the applicable end user consent state for this web app across all gtag references once Firebase Analytics is initialized.