-
Notifications
You must be signed in to change notification settings - Fork 57
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: customize localStorage key for analytics consents #302 #310
feat: customize localStorage key for analytics consents #302 #310
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.
Thanks!
LGTM beside thoses few points.
finalityDescription, | ||
personalDataPolicyLinkProps, | ||
consentCallback, | ||
localStorageKeyPrefix |
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.
localStorageKeyPrefix | |
localStorageKeyPrefix = defaultLocalStorageKeyPrefix |
const localStorageKey = `${ | ||
localStorageKeyPrefix ?? defaultLocalStorageKeyPrefix | ||
} ${finalities.join("-")}`; |
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.
const localStorageKey = `${ | |
localStorageKeyPrefix ?? defaultLocalStorageKeyPrefix | |
} ${finalities.join("-")}`; | |
const localStorageKey = `${localStorageKeyPrefix} ${finalities.join("-")}`; |
FooterPersonalDataPolicyItem, | ||
consentLocalStorageKey: localStorageKey |
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.
FooterPersonalDataPolicyItem, | |
consentLocalStorageKey: localStorageKey | |
FooterPersonalDataPolicyItem, | |
"consentLocalStorageKey": localStorageKey |
Will you actually use this value?
Because if there is no identified usecase for it no need to increace the API surface.
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.
Yeah so I see, you exported it for Storybook.
The storybook needs shouldn't affect the API.
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 after thinking, the use case we have won't be covered by just returning the computed key. So, I removed it from the return.
}, | ||
|
||
/* | ||
This optional parameter let's you personalise the key that is used to store user's consents in the localStorage. | ||
The default value is "${defaultLocalStorageKeyPrefix}" | ||
*/ | ||
"localStorageKeyPrefix": "company-name/app-name" |
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'd like to keep this undocumented.
It's a niche thing that is relevent in your usecase but for most app it won't be.
Pepole don't read documentation. If they see the doc is a little bit too long they'd rather implement their own solution.
So let's keep only the essential info.
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, I removed all mentions in the documentation.
Thanks! |
Hi! Here's my attempt to customise the localStorage key for analytics consents #302.
This PR :
localStorageKeyPrefix
parameter to thecreateConsentManagement
functionlocalStorageKeyPrefix
, renamed todefaultLocalStorageKeyPrefix
, as the default prefixconsentLocalStorageKey
(the calculated key) to the returned object of thecreateConsentManagement
function, so that it can be used by later by the user