fix: cookies functionality, hyperlinks and shown appropriate content in footer #1079
Conversation
github-actions
bot
added
🐞 bug
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @YuvarajSingh-0, thank you for raising a pull request.
Currently, the pull request is marked as https://github.com/MilanCommunity/Milan/labels/%F0%9F%91%B7%F0%9F%8F%BB%E2%80%8D%E2%99%82%EF%B8%8F%20status%3A%20awaiting%20triage which means that work for this issue is on hold and we are waiting for the maintainers/owner to review it and provide you with feedback/suggestions to proceed further.
Feel free to reach out to Tamal on Twitter, or drop a mail at tamalcodes@gmail.com if you think that this pull request is of critical priority.
Give us a ⭐ to show some support
Happy OpenSource 🚀
github-actions
bot
added
the
deployed
|
@tamalCodes Is this good? |
There was a problem hiding this comment.
Hi @YuvarajSingh-0 , i don't think you need to change any backend code OR the cookie name. The isLoggedIn cookie is being set and cleared by the backend itself - there's a logout issue because of the domain property maybe which I am working on actively.
However, with context to this issue, the isLoggedIn property is accessible in the code, look at the Navbar for example. Please use that cookie itself without further modifying any server-side code OR cookie name for the same.
Waiting for the changes in the PR !
tamalCodes
changed the title
tamalCodes
added
area/logic
|
@tamalCodes You mentioned the backend clears the isLoggedIn cookie but it doesn't which also raises issues with sign in and signup button visibility after logging out, but as you mentioned you are working on it, I am not going to modify anything in backend. Let me modify the code to create new PR |
Co-authored-by: Tamal Das <tamalcodes@gmail.com>
Co-authored-by: Tamal Das <tamalcodes@gmail.com>
There was a problem hiding this comment.
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.
So, you want the left image to look like the right one.... Right?
There was a problem hiding this comment.
Correct. You can check the current code that we have for more reference.
There was a problem hiding this comment.
What should I replace in place of 'usertype' cookie? (in line number 28 in HomeBanner.jsx)
There was a problem hiding this comment.
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.
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 name usertype and all.
Apart from these, the footer section is good to merge. But one thing, While logging out, Both cookies isLoggedIn and Token should be deleted, failing either one may result in inconsistent results while conditional rendering. Because footer only checks isLoggedIn cookie, so even if there is no Token cookie but there is isLoggedIn cookie, it still considers it as logged in.
There was a problem hiding this comment.
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 the Token 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.
|
This pull request has been deployed to Vercel.
|
There was a problem hiding this comment.
Amazing work 🎉
The changes look good to me and will be merged soon.
Do follow Tamal for more Opensource fun projects and don't forget to drop a star so that you get updated about our latest releases (we will tag you and mention your work) and also a shoutout on social media (LinkedIn and Twitter) !
Happy Opensource 🚀.
tamalCodes
added
status: ready ✅
|
It's been great contributing this project, I apologise for the delay in resolving this small issue |
closes #1070
👷🏻 Changes made
Corrected cookie names all around code and fixed backend code that fixes this issue.
📸 Screenshots
Before Logging in 👇
After logging in 👇
