Skip to content
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

Merged

Conversation

ocruze
Copy link
Contributor

@ocruze ocruze commented Sep 16, 2024

Hi! Here's my attempt to customise the localStorage key for analytics consents #302.

This PR :

  • adds a localStorageKeyPrefix parameter to the createConsentManagement function
  • keeps the original localStorageKeyPrefix, renamed to defaultLocalStorageKeyPrefix, as the default prefix
  • adds consentLocalStorageKey (the calculated key) to the returned object of the createConsentManagement function, so that it can be used by later by the user
  • updated the story

Copy link
Collaborator

@garronej garronej left a 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
localStorageKeyPrefix
localStorageKeyPrefix = defaultLocalStorageKeyPrefix

Comment on lines 42 to 44
const localStorageKey = `${
localStorageKeyPrefix ?? defaultLocalStorageKeyPrefix
} ${finalities.join("-")}`;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const localStorageKey = `${
localStorageKeyPrefix ?? defaultLocalStorageKeyPrefix
} ${finalities.join("-")}`;
const localStorageKey = `${localStorageKeyPrefix} ${finalities.join("-")}`;

Comment on lines 109 to 110
FooterPersonalDataPolicyItem,
consentLocalStorageKey: localStorageKey
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Comment on lines 125 to 131
},

/*
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"
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@garronej
Copy link
Collaborator

Thanks!

@garronej garronej merged commit bb2e83e into codegouvfr:main Sep 17, 2024
6 checks passed
@ocruze ocruze deleted the feat/personalise-analytics-localstorage-key branch September 17, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants