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.

Sign up for GitHub

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

Jump to bottom

1171 cookie consent clean up #1199

Merged
merged 17 commits into from
Nov 6, 2024
Merged

1171 cookie consent clean up #1199

merged 17 commits into from
Nov 6, 2024

Conversation

snmln
Copy link
Contributor

@snmln snmln commented Oct 18, 2024 •
edited
Loading

Related Ticket: #1171

Description of Changes

  • Relocating cookie retrieval, validation, and creation functionality from layoutRoot to cookieConsentForm
  • Implementing session item to track when a session has begun for each individual user and only run validation and retrieval functionality on session start and when a response has not been recorded.
  • Adjusting Banner content to accept markdown in config props.

Notes & Questions About Changes

Validation / Testing

Clear out all cookies in your local machine to do so: open dev tools -> navigate to application tab-> navigate to cookies in the left panel -> right click clear

  • After clearing your existing site cookies, navigate to any page. Click on Decline. Navigate to another page and you should not see the cookie consent form component.
  • Clearing your existing site cookies again, navigate to any page. Click on Accept. Navigate to another page and you should not see the cookie consent form component.
  • Clearing your existing site cookies again, navigate to any page. Click on X. Navigate to another page and you should see the cookie consent form component rerendered.

@netlify Netlify
Copy link

netlify bot commented Oct 18, 2024 •
edited
Loading

✅ Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 598ac2a
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/672a6c6f1deec600085bb110
😎 Deploy Preview https://deploy-preview-1199--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.

@snmln snmln linked an issue Oct 18, 2024 that may be closed by this pull request
Cookie Consent - Clean up #1171
Closed
3 tasks
@snmln snmln marked this pull request as ready for review October 23, 2024 19:09
@dzole0311
Copy link
Collaborator

dzole0311 commented Oct 24, 2024 •
edited
Loading

I noticed an issue when navigating between routes: the cookie consent temporarily disappears and then again reappears. Is this related to the relocation of the cookie functionality?

The issue persists when reloading the page as well (without any interaction with the cookie banner).

I've recorded a video with the behavior below:

Screen.Recording.2024-10-24.at.13.50.32.mov

@hanbyul-here
Copy link
Collaborator

👋 Apologies for the review being late. I am somehow having trouble running this branch locally. I doubt that it is something this pr iontroduced since our dev environment has been unstable time to time. But in case, can you confirm me that you have no problem running this branch on local ?

@snmln
Copy link
Contributor Author

snmln commented Nov 1, 2024 •
edited
Loading

Hey @hanbyul-here no problem, odd I was able to run it previously but now the system is getting hung up on the bundling step locally.

Actually rebasing seemed to have corrected the issue.

@hanbyul-here
Copy link
Collaborator

hanbyul-here commented Nov 4, 2024 •
edited
Loading

@snmln I did rebase main, but I still have a problem running a local instance. trying to figure out what is going on now. - When I comment out CookieContent related logic, I can run it - something is getting infinitely re-rendered.

hanbyul-here and others added 7 commits November 4, 2024 13:11
@hanbyul-here
Fix utility function for cookie
a8022c5
@hanbyul-here
Move up path name logic, add cookie util test
7061657
@snmln
Cookie consent render logic (#1240)
0b1a732 
This pr fixes infinite rerendering issue (it was from the parsing error
of cookie value - so I added a unit test to make sure this works. - I
used the solution from this post, feel free to edit if you see any
improvement
https://stackoverflow.com/questions/5142337/read-a-javascript-cookie-by-name)
- also feel free to edit the test, I mainly put it for my dev purpose.

I removed currenwindowurl logic, replaced it with pathname passed from
the layout root. I started making this change and then realized that
this is not the cause of infinite rendering problem, but I think it is
probably safer to depend on the pathname change than location.href since
href can include so many things like query parameters?
@hanbyul-here
Do not use session
6d5ad43
@hanbyul-here
Use debounced function
368c2be
@hanbyul-here
Fix test
2682238
@snmln
Simplify logic for cookie consent (#1241)
598ac2a 
First of all, I apologize that I could not make this suggestion as a
part of review. It is still challenging for me to make a suggestion
without actually touching the code.

This PR handles several things. The first to fix is cookieconsent form
gradually disappears on the first page load. The second is not depending
on session storage - mainly because I feel like it was diverging the
logic flow - but if I am missing anything, please feel free to close
this. The third is moving out the utility function outside of the
component, so it doesn't have to be declared as a dependency of
useEffect hook.
hanbyul-here
hanbyul-here approved these changes Nov 5, 2024
View reviewed changes
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!

dzole0311
dzole0311 approved these changes Nov 6, 2024
View reviewed changes
Copy link
Collaborator

@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.

Re-tested the cookie consent across routes and page reloads: it renders as expected.

@snmln snmln merged commit d2bc0e5 into main Nov 6, 2024
9 checks passed
@snmln snmln deleted the 1171-Cookie-Consent---Clean-up branch November 6, 2024 12:17
@AliceR AliceR mentioned this pull request Nov 11, 2024
Merged
AliceR added a commit that referenced this pull request Nov 12, 2024
@AliceR
[RELEASE] v5.10.0 (#1250)
6e929e8 
## 🎉 Features
* Card image/description independent of hero image/description by
@dzole0311 in #1244

## 🚀 Improvements
* Cookie consent code cleanup by @snmln in
#1199 , and @hanbyul-here in
#1240 , and @hanbyul-here in
#1241
* Add ADR about design system change by @j08lue in
#890
* Update condition to run playwright tests on release branches by
@dzole0311 in #1228
* Update STYLE_GUIDE.md by @AliceR in
#1227
* Fix lint configuration by @AliceR in
#1219
* Add tests for the AOI feature specification by @AliceR in
#1216
* Set data catalog filters to be closed by default by @vgeorge in
#1243
* Update tsconfig and make nav interfaces exposable for consumption by
@sandrahoang686 in #1223

## 🐛 Fixes
* Hotfix to hide the external link badge from cards by @dzole0311 in
#1231

## New Contributors
* @vgeorge made their first contribution in
#1243

**Full Changelog**:
v5.9.0...v5.10.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dzole0311 dzole0311 dzole0311 approved these changes

@hanbyul-here hanbyul-here hanbyul-here approved these changes

@AliceR AliceR Awaiting requested review from AliceR

@sandrahoang686 sandrahoang686 Awaiting requested review from sandrahoang686

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cookie Consent - Clean up
3 participants
@snmln @dzole0311 @hanbyul-here