Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix: cookies functionality, hyperlinks and shown appropriate content in footer #1079
fix: cookies functionality, hyperlinks and shown appropriate content in footer #1079
Changes from 3 commits
9c044fa
50df26b
6690088
767867e
596a8f8
6d28cd6
026e382
d8f67fb
5b6b106
00293ce
785f6e1
1be2776
002ab15
8257102
81613a8
14f9025
fcafac1
72b4be2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It looks like you have resolved the conflicts in the wrong manner. You overwrote the incoming changes (the changes that I made) with the changes of the current branch (the changes that you had previously).
That is a reason why some UI has also changed. Take a look at this screenshot below.
The left image is the changes that are made by you. The right one is what is currently deployed.
Please fix this.
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.
So, you want the left image to look like the right one.... Right?
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.
Correct. You can check the current code that we have for more reference.
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.
What should I replace in place of 'usertype' cookie? (in line number 28 in HomeBanner.jsx)
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 think starting from here it would be the best to keep the changes as it is.
I previously updated the code for the home banner about what to show in the buttons on instances where the user is not logged in, the club is logged in OR the user is logged in. So I think we can maybe ignore the changes for the
HomeBanner.jsx
file.I also see that you have optimized the code for the conditional rendering, thanks for that.
Can you confirm for me if the Login and Logout-based rendering functionality is working perfectly for the footer and stuffs ? IF yes we can merge the same.
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.
Yes, It's Working fine, I've checked it now.
There was also an issue with login trigger while logging in i.e., the login functionality is not hitting the correct backend url for login, I fixed it in ApiEndpoints.js file. And also upon clicking the profile icon after logging in, due to this line the redirecting url does not exist due to no
usertype
cookie. But I think that part of code depends on backend, like setting cookie with nameusertype
and all.Apart from these, the footer section is good to merge. But one thing, While logging out, Both cookies
isLoggedIn
andToken
should be deleted, failing either one may result in inconsistent results while conditional rendering. Because footer only checksisLoggedIn
cookie, so even if there is noToken
cookie but there isisLoggedIn
cookie, it still considers it as logged in.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.
The Login and the
usertype
works fine when normally logging in. Google authentication won't work just yet - so please stay tuned. You can learn more about that in this issue.The
token
is an HTTP-only cookie - so you won't be able to access the cookie from the front end. So checking for theToken
cookie makes no sense. Currently, we might have some glitch in the backend where the logout function fails to clear the cookies in production - but it should work properly in the local.