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

Allow users to browse before having to signup/login #278

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

mnixo
Copy link
Contributor

@mnixo mnixo commented Jul 5, 2024

Description

Closes #276
Allows anonymous users (unauthenticated) to navigate the app and browse listed items.
When they try to access restricted pages (add item, conversations and profile), they are prompted to login.
The app landing page is now the home page.

Files changed

The landing page is updated to simply redirect to the home page.
Add item, conversations and profile pages are updated to redirect to the login page when the user is not logged in.
Item page is updated to hide the "MESSAGE" button when the user is not logged in.
A new Cypress E2E test file is added and additional page objects and fixtures are created/updated.

UI changes

276.mp4

Tests

Tests added to cover the following scenarios for anonymous user navigation:

  • Is redirected from landing page to home page.
  • Can browse items.
  • Can see about page.
  • Add item redirects to login.
  • Message redirects to login.
  • Profile redirects to login.

Copy link

netlify bot commented Jul 5, 2024

👷 Deploy request for cool-creponne-3e1272 pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 5dcee5b

Copy link
Contributor

@camelPhonso camelPhonso left a comment

Choose a reason for hiding this comment

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

Really great 👌 thanks !

Thanks for adding to the E2E tests for this as well.

I'm requesting a change as I think there's a really good chance here to correct some weird routes we've inherited but the code all looks awesome.

</main>
</>
);
redirect('/home-page');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would probably make sense to use this opportunity to correct a bit of a weird design decision that was made at the start of the project.

Since people will be starting from the home-page route we should probably just make that the actual home and move it to here.

This would mean correcting a few redirects throughout the codebase but now is probably the perfect opportunity to do it.

So that would leave us without a /home-page route and its contents would live in / instead. All others stay the same.

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.

Allow users to browse before having to signup/login
2 participants