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

Creating cookie consent component #1127

Merged
merged 40 commits into from
Sep 26, 2024
Merged

Creating cookie consent component #1127

merged 40 commits into from
Sep 26, 2024

Conversation

snmln
Copy link
Contributor

@snmln snmln commented Aug 26, 2024

Related Ticket: Cookie Consent Form

Description of Changes

USWDS Alert component used as a cookie consent form. This form should render on any page of the site if a user has not consented to the use of cookies. If a user Accepts Cookies or Declines Cookies a cookie will be made to catalog the response by the user. The cookie will expire after 3 months and the form will re-render for the user.

Notes & Questions About Changes

Figma Designs: https://www.figma.com/design/xaZSp74DKFGYm0k2w2BbVb/Shared-VEDA-file?node-id=0-1&t=wGNpe3xrUyLTU1pB-1

CookieConsent form is stored behind a feature flag to allow for each instance to opt in to leveraging the component. To turn the CookieConsentForm ON in the veda-ui/.env set COOKIE_CONSENT_FORM='TRUE'

To change contents of the consent form change the contents of cookieConsentForm within the veda-ui/mock/veda.config.js to reflect desired content. To add link in the content, it must be added in the following format [Link text](URL)

Example prop: 'We use cookies to enhance your browsing experience and to help us understand how our website is used. These cookies allow us to collect data on site usage and improve our services based on your interactions. To learn more about it, see our [Privacy Policy](https://www.nasa.gov/privacy/#cookies)'

Validation / Testing

Run unit test CookieConsent.spec.js. To test in browser, run locally navigate to browser cookies you should see the following values for specific interactions:

  • Decline Cookie: {"responded":true,"answer":false}
  • Accept Cookie: {"responded":true,"answer":true}
  • Close out: {"responded":false,"answer":false}

Copy link

netlify bot commented Aug 26, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 3bbd537
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/66f439c1b754f000082a3581
😎 Deploy Preview https://deploy-preview-1127--veda-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@aboydnw
Copy link
Contributor

aboydnw commented Aug 27, 2024

I think this looks great. Seeing this for me raises two questions:

  1. Should we also include a close button?
  2. After somebody takes any of these actions, what happens next time they visit the site? Would they see the alert again or do we have a way of recognizing that they already answered the form?

Also, I know it's just content and not what you asked for feedback on, but should the Privacy Policy link open in a new tab?

@snmln
Copy link
Contributor Author

snmln commented Aug 27, 2024

I think this looks great. Seeing this for me raises two questions:

  1. Should we also include a close button?
  2. After somebody takes any of these actions, what happens next time they visit the site? Would they see the alert again or do we have a way of recognizing that they already answered the form?

Also, I know it's just content and not what you asked for feedback on, but should the Privacy Policy link open in a new tab?

We can definitely introduce a close out button in the interface for those that would rather not respond. It does then prompt the question, do we interpret a close out of the interface as a decline or an non-answer and continue to collect the cookies from their session. Based on the Nasa.gov data privacy site (which is somewhat vague) I would interpret it as a non-answer and continue collecting:

Each NASA site using persistent cookies identifies itself as doing so. If you do not wish to have session or persistent cookies stored on your machine, you can turn them off in your browser. However, this may affect the functioning of some NASA Web sites.

As for the next time the user comes to the site, it depends on what type of cookie we are using if they are persistent cookies the user is good to go, no reoccurring interface on their end unless they delete their browsing history. We can grab that information via their browser during each session. As for opening a link in a new tab, yes the privacy link should open in a new tab, that is something we can note in the acceptance criteria once we move to development.

@aboydnw
Copy link
Contributor

aboydnw commented Aug 27, 2024

Based on the Nasa.gov data privacy site (which is somewhat vague) I would interpret it as a non-answer and continue collecting

That seems reasonable to me 👍

Copy link
Contributor

@dzole0311 dzole0311 left a comment

Choose a reason for hiding this comment

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

The POC looks great @snmln, welcome to the project!

app/scripts/components/common/cookie-consent/index.scss Outdated Show resolved Hide resolved
@snmln snmln marked this pull request as ready for review September 3, 2024 21:24
@snmln snmln marked this pull request as draft September 3, 2024 21:25
@snmln snmln changed the title DESIGN POC | Creating cookie consent component POC Creating cookie consent component Sep 5, 2024
@snmln snmln marked this pull request as ready for review September 5, 2024 14:55
Copy link
Collaborator

@hanbyul-here hanbyul-here left a comment

Choose a reason for hiding this comment

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

Thanks for working on it, I left some comments that hopefully helps you. + It will be good if you can review the code and clean up a bit (remove unnecessary comments etc.)

@@ -0,0 +1,25 @@
#cookie-consent{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you had a chance to familiarize yourself with utility functions (such as https://designsystem.digital.gov/utilities/display/, https://designsystem.digital.gov/design-tokens/z-index/ ) in USWDS? I think a lot of this custom style can be replaced with the USWDS utility class - and this will also give us some coherence on the units.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did, but wasn't familiar with all of the available tokens. I will get those adjusted.

@@ -35,17 +43,58 @@ const PageBody = styled.div`
`;

function LayoutRoot(props: { children?: ReactNode }) {
const useConsentForm = checkEnvFlag(process.env.COOKIE_CONSENT_FORM);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is still using env variable. And I think your intention is to depend on veda config instead of env var?

app/scripts/components/common/layout-root/index.tsx Outdated Show resolved Hide resolved
app/scripts/components/common/layout-root/index.tsx Outdated Show resolved Hide resolved
copy,
onFormInteraction
}: CookieConsentProps) => {
const [cookieConsentResponded, SetCookieConsentResponded] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to learn more about where to save cookie consent prefrence. We have been saving the user preference on local storage so far - can you leave some notes about why we are using cookie for this case? Moreover, when user says no to cookie usage, can we save this info to cookie at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can change this to local storage if that is the existing pattern across the system.

Based on what I could find through my original research, the prevailing pattern is to store the consent in a cookie since they require an expiration associated with the cookies. Allowing for redundancy when a user clears their cache and cookies they will be re-prompted with cookie consent interface. Because of the temporary nature it appears to be the primary storage method (while contradictory to the implied practice).

hanbyul-here and others added 7 commits September 20, 2024 17:01
**Related Ticket:** [_{link related ticket
here}_](#1127)

### Description of Changes
We already have a mechanism to parse markdown to html bts. It is highly
likely that users will try to use different markdown syntax if they see
the link syntax. So here, I made parcel resolver to compile the
cookieconsent form to html beforehand. I only did it for copy, but some
questions are
- does title also accept markdown?
- should we have some kinds of prefixes to note that this is a markdown?
(we use ::markdown prefix for mdx files, but this is configuration so it
can be a bit different story.
…when close btn is pressed (#1165)

I removed some styles that are not necessary for cookieconsent. I will
put my reasoning with inline comments.

(Lots of changes are nitpicks, but I thought it might be a good
opportunity to onboard you on some practices that are not well
documented)
@hanbyul-here
Copy link
Collaborator

hanbyul-here commented Sep 24, 2024

As I mentioned in stand-up let's merge this after v5.8.0 goes out. But there are several improvements we can make
(This PR is already pretty big, so these things can be followed up as separate PRs.)

  1. Currently, the scripts that check cookies run every time a new page loads which I am not sure if it is necessary.

  2. (might be related to 1.) I feel like Layout component is doing too much of cookie-related works, which should not be a part of layout's job. It might be slightly complicated in real life since it is layout's job to decide whether to render a specific component or not. But maybe there is something in the middle ground.

  3. (This is partially my fault that I rushed the change) Banner and CookieConsent expects different forms for its content, Banner expects html and CookieConsent expects markdown. I am fine with either of them, but we should consolidate them.

@aboydnw
Copy link
Contributor

aboydnw commented Sep 24, 2024

@snmln can you create a ticket in veda-ui to represent this followup work? Might be good practice, and I can help you fill out any metadata on the ticket if you have questions

@snmln
Copy link
Contributor Author

snmln commented Sep 24, 2024

@snmln can you create a ticket in veda-ui to represent this followup work? Might be good practice, and I can help you fill out any metadata on the ticket if you have questions

Of course!

@snmln
Copy link
Contributor Author

snmln commented Sep 24, 2024

@snmln can you create a ticket in veda-ui to represent this followup work? Might be good practice, and I can help you fill out any metadata on the ticket if you have questions

Of course!

@aboydnw I have gone ahead and create 2 tickets to reflect the work:
Cookie Consent - fast follow
Cookie Consent - Clean up

Please let me know if I need to adjust any tagging or anything else for these tickets.

@aboydnw
Copy link
Contributor

aboydnw commented Sep 24, 2024

Thanks @snmln . I wonder if the action of merging this PR is covered by #953 that is In Review right now, then we wouldn't need #1170 and can schedule #1171 for an upcoming sprint

What do you think?

@snmln
Copy link
Contributor Author

snmln commented Sep 25, 2024

@aboydnw I agree with that plan! I will go ahead and delete #1170

Copy link
Collaborator

@hanbyul-here hanbyul-here left a comment

Choose a reason for hiding this comment

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

I found an error that I didn't use the constant for setting cookie ! I pushed a fix for it: 9356b87

I think this should be an ok status to merge, but I touched it too much at this point 😅 so please take time to test the functionality before merging.

@snmln snmln merged commit 8967eaf into main Sep 26, 2024
8 checks passed
@snmln snmln deleted the Cookie-consent-form-POC branch September 26, 2024 13:25
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.

5 participants