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

Issue 701: cookie banner re-rendering #704

Merged
merged 2 commits into from
Jun 25, 2024

Conversation

JT0Y
Copy link

@JT0Y JT0Y commented Jun 23, 2024

#701

This resolves the cookie banner re-rendering by recording/checking the user's local storage.

The issue can be recreated by:

  • running localhost
  • take action on cookie banner, either accept or decline
  • refresh tab, or open new tab and the cookie banner should re-render
Screenshot 2024-06-22 at 7 02 37 PM Screenshot 2024-06-22 at 7 02 47 PM

Copy link

vercel bot commented Jun 23, 2024

Someone is attempting to deploy a commit to the Clean and Green Philly Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Collaborator

@CodeWritingCow CodeWritingCow left a comment

Choose a reason for hiding this comment

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

Hi @JT0Y, thanks for contributing! I tested your code in my local environment. When I accept cookies and then open the site in a new browser tab, the cookie banner no longer appears.

However, our GitHub repo's Frontend PR Checks are failing due to this error: ReferenceError: localStorage is not defined. I'm getting the same error in my localhost ... See screenshot below:

Screen Shot 2024-06-23 at 12 00 31 AM

Can you update the PR to resolve this? Thanks!

@JT0Y
Copy link
Author

JT0Y commented Jun 23, 2024

However, our GitHub repo's Frontend PR Checks are failing due to this error: ReferenceError: localStorage is not defined. I'm getting the same error in my localhost ... See screenshot below:
Can you update the PR to resolve this? Thanks!

yes, I came across this issue and was a bit confused on the issue. I am not familiar with SSR and this environment. But I have additional checks that check if this code is running on a client or not. I did some quick googling on this and this seems to be a well documented issue. Please let me know if this is an unusual pattern, as I am new to next.js & SSR, this does feel strange to me.

I have tested this and am still able to dismiss the banner and not receiving this issue as well:

app-index.js:35 Warning: Prop `className` did not match. Server: "md:flex fixed inset-x-0 bottom-0 z-20 justify-between bg-blue-200 p-4 sm:p-6" Client: "hidden"

@CodeWritingCow
Copy link
Collaborator

@JT0Y I ran the updated PR locally, and the localStorage error is no longer appearing.

Your solution looks sound. By checking if window.localStorage exists, we can confirm whether the code is running on the client side. Also, React useEffect hooks run only on the client side. So when we execute localStorage code inside those hooks, we'll avoid those undefined errors. (I'm also new to SSR, so I had to read up a bit on it too.)

Thanks for your work!

@JT0Y
Copy link
Author

JT0Y commented Jun 25, 2024

@CodeWritingCow No worries and thanks for double checking!
Do I need to do anything from here? My understanding from the contributing doc is that I just need to wait for a codeowner to merge it into staging and then close this PR?

@CodeWritingCow
Copy link
Collaborator

@JT0Y I'll take it from here. Will merge and close this PR!

@CodeWritingCow CodeWritingCow merged commit de448ea into CodeForPhilly:staging Jun 25, 2024
1 of 2 checks passed
@CodeWritingCow CodeWritingCow added bug Something isn't working frontend labels Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants