-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
feat: Update UI of signup / login pages #16710
Conversation
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details:
|
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (09/19/24)1 reviewer was added to this PR based on Keith Williams's automation. "Add foundation team as reviewer" took an action on this PR • (09/20/24)1 reviewer was added to this PR based on Keith Williams's automation. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
apps/web/pages/signup.tsx
Outdated
{isGoogleLoginEnabled ? ( | ||
<Button | ||
color="secondary" | ||
disabled={!!formMethods.formState.errors.username || premiumUsername} |
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 removed this because this will look buggy now that the email form and google sign-in button are not on the same page. Lmk what you think cc @sean-brydon
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.
Seems fine to me!
<div className="my-8"> | ||
<div className="relative flex items-center"> | ||
<div className="border-subtle flex-grow border-t" /> | ||
<span className="text-subtle mx-2 flex-shrink text-sm font-normal leading-none"> | ||
{t("or").toLocaleLowerCase()} | ||
</span> | ||
<div className="border-subtle flex-grow border-t" /> | ||
</div> | ||
</div> |
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.
new code added
<div className="space-y-3"> | ||
{isGoogleLoginEnabled && ( | ||
<Button | ||
color="primary" |
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.
style change
apps/web/pages/signup.tsx
Outdated
</Form> | ||
</div> | ||
)} | ||
{!displaySignupForm && ( |
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.
Buttons Container starts here
apps/web/pages/signup.tsx
Outdated
orgSlug | ||
? `${getOrgFullOrigin(orgSlug, { protocol: true }).replace(URL_PROTOCOL_REGEX, "")}/` | ||
: `${process.env.NEXT_PUBLIC_WEBSITE_URL.replace(URL_PROTOCOL_REGEX, "")}/` | ||
{displaySignupForm && ( |
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.
Form Container starts here
@@ -39,15 +38,12 @@ interface LoginValues { | |||
} | |||
|
|||
const GoogleIcon = () => ( | |||
<img className="text-subtle mr-2 h-4 w-4 dark:invert" src="/google-icon.svg" alt="" /> | |||
<img className="text-subtle mr-2 h-4 w-4 dark:invert" src="/google-icon-colored.svg" alt="" /> |
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 confirmed with Peer. No issues with icon licensing
E2E results are ready! |
8c76170
to
9f2df85
Compare
apps/web/pages/signup.tsx
Outdated
} | ||
className={classNames( | ||
"w-full justify-center rounded-md text-center", | ||
formMethods.formState.errors.username ? "opacity-50" : "" |
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 removed this for the same reason as above
apps/web/pages/signup.tsx
Outdated
if (prepopulateFormValues?.username) { | ||
// If username is present we save it in query params to check for premium | ||
searchQueryParams.set("username", prepopulateFormValues.username); | ||
localStorage.setItem("username", prepopulateFormValues.username); | ||
} |
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 introduced changes here for username
@@ -10,7 +10,7 @@ import { expectInvitationEmailToBeReceived } from "./team/expects"; | |||
|
|||
test.describe.configure({ mode: "parallel" }); | |||
|
|||
test.describe("Signup Flow Test", async () => { | |||
test.describe("Email Signup Flow Test", async () => { |
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.
Because this test is specific for email signup flow test. We should create a new test for the new /signup page. I created a linear ticket here: https://linear.app/calcom/issue/CAL-4366/create-a-new-test-for-the-new-signup-page
UPDATE: Just did the job in this PR
Next step (a separate PR): Look into what I need to change in order for org/team invites work with Google Signin; For now, the email form will appear when users open links for org/team invites |
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.
We should revert the yarn.lock changes
What does this PR do?
disabled
prop orsearchQueryParams
that is added to Google auth url no longer depend on sign-up form values)Mandatory Tasks (DO NOT REMOVE)
How should this be tested?