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

fix: force user to login only on account dependent page #1045

Merged
merged 5 commits into from
Dec 11, 2024

Conversation

wa0x6e
Copy link
Contributor

@wa0x6e wa0x6e commented Dec 11, 2024

Summary

Closes: https://github.com/snapshot-labs/workflow/issues/325

This PR will force prompt users to login only page depending on the current wallet (except /home).

  • Remove the force login modal on the /home page, will redirect users to /explore page instead
  • Add the force login modal on /notifications and /settings/* page, since these page make sense only if wallet is connected, and accessible only with direct link

Also fix a minor issue where logging out while on the /notifications page was not clearing the existing notifications

How to test

  1. Disconnect your wallet
  2. Go to /home
  3. You should be redirected to /explore
  4. Go to /notifications or /settings
  5. You should see the login modal
  6. Login
  7. Go to /home
  8. You should see the /home

@wa0x6e wa0x6e changed the title Fix-do-not-force-login-on-home fix: force user to login only on account dependent page Dec 11, 2024
@wa0x6e wa0x6e requested a review from ChaituVR December 11, 2024 04:49
@wa0x6e wa0x6e marked this pull request as ready for review December 11, 2024 04:49
Copy link
Member

@ChaituVR ChaituVR left a comment

Choose a reason for hiding this comment

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

tAck

[() => web3.value.account, () => web3.value.authLoading],
([account, authLoading]) => {
if (!account && !authLoading) {
modalAccountWithoutDismissOpen.value = true;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe better to redirect to home/explore page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My reasoning is as follow:

user who wants these page have a direct link (bookmarks, etc…), and want their content. If not logged in, they would want to login, else it would not make sense to access these pages.

/home is different, as there’s a link from the landing page, and anyone can go there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bonustrack wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Let's merge this for now

Copy link
Member

Choose a reason for hiding this comment

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

Make sense, I'll merge this

@bonustrack bonustrack merged commit be5fb73 into master Dec 11, 2024
2 checks passed
@bonustrack bonustrack deleted the fix-do-not-force-login-on-home branch December 11, 2024 17:05
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.

3 participants