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.

Sign up for GitHub

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

Jump to bottom

feat: uniform post page login #4046

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

feat: uniform post page login #4046

wants to merge 25 commits into from

Conversation

AmarTrebinjac
Copy link
Contributor

@AmarTrebinjac AmarTrebinjac commented Jan 6, 2025 •
edited
Loading

Changes

This task is named "uniform post page login", but the changes here affect more than just post page as I directly modified the "LoginButton" that is used both in the CustomAuthBanner (mobile banner) and Navbar.
Chris mentions here that only login flow should be inside /onboarding to be able to leverage all onboarding steps. So unless we want the Navbar to pop the modal on other pages than /posts/, I say we keep it this way

Events

Did you introduce any new tracking events?

Experiment

Did you introduce any new experiments?

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

Preview domain

https://mi-696.preview.app.daily.dev

@vercel Vercel
Copy link

vercel bot commented Jan 6, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
daily-webapp ✅ Ready (Inspect) Visit Preview Jan 21, 2025 4:59pm
1 Skipped Deployment
Name Status Preview Updated (UTC)
storybook ⬜️ Ignored (Inspect) Jan 21, 2025 4:59pm

@AmarTrebinjac AmarTrebinjac changed the title Mi 696 feat: Uniform post page login Jan 6, 2025
@AmarTrebinjac AmarTrebinjac marked this pull request as ready for review January 7, 2025 09:49
@AmarTrebinjac AmarTrebinjac requested a review from a team as a code owner January 7, 2025 09:49
rebelchris
rebelchris approved these changes Jan 20, 2025
View reviewed changes
Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

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

For me looks ok like this :)
Maybe just wait for 1 more review.

omBratteng
omBratteng reviewed Jan 20, 2025
View reviewed changes
Copy link
Member

@omBratteng omBratteng left a comment

Choose a reason for hiding this comment

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

When I click on Login or Signup while I'm browsing the public feeds, or on a post page, I am not able to navigate back in my history, it sends me back to onboarding page.

And it would be nice to show the login form after I click on login button, so that I don't have to click on it again

@AmarTrebinjac
Copy link
Contributor Author

AmarTrebinjac commented Jan 20, 2025 •
edited
Loading

When I click on Login or Signup while I'm browsing the public feeds, or on a post page, I am not able to navigate back in my history, it sends me back to onboarding page.

And it would be nice to show the login form after I click on login button, so that I don't have to click on it again

I'm not so sure if the login leading to the registration form is a good idea. That button is present a lot of places where the banner is not, so taking them straight to registration may give the impression that registering by email/password is the only way.

I'll have a look at the navigation

capJavert
capJavert reviewed Jan 20, 2025
View reviewed changes
Copy link
Contributor

@capJavert capJavert left a comment

Choose a reason for hiding this comment

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

Looks good to me, have some minor comments.

Not sure of all of the cases that should be tested with this flow, if they are specified I can try testing e2e as well.

packages/shared/src/components/auth/AuthenticationBanner.tsx Outdated Show resolved Hide resolved
packages/shared/src/contexts/AuthContext.tsx Outdated
Comment on lines 152 to 159
const params = new URLSearchParams(globalThis?.location.search);
const triggerParam = params.get('authTrigger');
if (loginState && loginState?.trigger !== triggerParam) {
if (!params.get('afterAuth')) {
params.set('afterAuth', window.location.pathname);
}
params.set('authTrigger', loginState.trigger);
router.push(`/onboarding?${params.toString()}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This repeats quite a few times so maybe extract it in helper? Non blocking

packages/webapp/pages/onboarding.tsx Outdated Show resolved Hide resolved
packages/webapp/pages/onboarding.tsx Outdated Show resolved Hide resolved
@omBratteng
Copy link
Member

When I click on Login or Signup while I'm browsing the public feeds, or on a post page, I am not able to navigate back in my history, it sends me back to onboarding page.
And it would be nice to show the login form after I click on login button, so that I don't have to click on it again

I'm not so sure if the login leading to the registration form is a good idea. That button is present a lot of places where the banner is not, so taking them straight to registration may give the impression that registering by email/password is the only way.

I'll have a look at the navigation

I mean when I click on
image
or on
image

I get sent to this page
image
instead of onboarding front page, where I have to click on login again before I see login form.
image

@AmarTrebinjac
Copy link
Contributor Author

Since we deleted the authModal completely and always redirect to onboarding now, I figured refactoring the showLogin would be a better option.

I think all the above comments should be solved now. I did some E2E testing myself and everything seems OK, but please do check it yourselves @capJavert and @omBratteng, the more the merrier :)

@omBratteng omBratteng changed the title feat: Uniform post page login feat: uniform post page login Jan 21, 2025
@omBratteng omBratteng requested a review from rebelchris January 21, 2025 17:00
omBratteng
omBratteng approved these changes Jan 21, 2025
View reviewed changes
Copy link
Member

@omBratteng omBratteng left a comment

Choose a reason for hiding this comment

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

LGTM.
Login works and redirects me to origin, but unable to test email signup because of turnstile blocking me I think 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omBratteng omBratteng omBratteng approved these changes

@ilasw ilasw Awaiting requested review from ilasw ilasw is a code owner automatically assigned from dailydotdev/web-team

@sshanzel sshanzel Awaiting requested review from sshanzel sshanzel is a code owner automatically assigned from dailydotdev/web-team

@nensidosari nensidosari Awaiting requested review from nensidosari nensidosari is a code owner automatically assigned from dailydotdev/web-team

@capJavert capJavert Awaiting requested review from capJavert

@rebelchris rebelchris Awaiting requested review from rebelchris

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AmarTrebinjac @omBratteng @rebelchris @capJavert