Skip to content

refactor(site): refactor login screen #10768

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

Merged
merged 2 commits into from
Nov 20, 2023
Merged

refactor(site): refactor login screen #10768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f89a440
refactor(site): refactor login screen
BrunoQuaresma Nov 17, 2023
3900c4a
Fix test
BrunoQuaresma Nov 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@
"wsconncache",
"wsjson",
"xerrors",
"xlarge",
"yamux"
],
"cSpell.ignorePaths": ["site/package.json", ".vscode/settings.json"],
Expand Down
4 changes: 4 additions & 0 deletions site/src/@types/mui.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ declare module "@mui/material/Button" {
interface ButtonPropsColorOverrides {
neutral: true;
}

interface ButtonPropsSizeOverrides {
xlarge: true;
Copy link
Member

Choose a reason for hiding this comment

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

Just making sure I understand the overrides here: is the true value here set up as a boolean property just because of MUI's syntax?

Basically, does having xlarge: true just mean that the type of the component's size prop becomes builtInType | "xlarge"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly!

}
}

declare module "@mui/system" {
Expand Down
29 changes: 0 additions & 29 deletions site/src/pages/LoginPage/LoginPage.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
} from "testHelpers/renderHelpers";
import { server } from "testHelpers/server";
import { LoginPage } from "./LoginPage";
import * as TypesGen from "api/typesGenerated";

describe("LoginPage", () => {
beforeEach(() => {
Expand Down Expand Up @@ -76,32 +75,4 @@ describe("LoginPage", () => {
// Then
await screen.findByText("Setup");
});

it("hides password authentication if OIDC/GitHub is enabled and displays on click", async () => {
const authMethods: TypesGen.AuthMethods = {
password: { enabled: true },
github: { enabled: true },
oidc: { enabled: true, signInText: "", iconUrl: "" },
};

// Given
server.use(
rest.get("/api/v2/users/authmethods", async (req, res, ctx) => {
return res(ctx.status(200), ctx.json(authMethods));
}),
);

// When
render(<LoginPage />);

// Then
expect(screen.queryByText(Language.passwordSignIn)).not.toBeInTheDocument();
await screen.findByText(Language.githubSignIn);

const showPasswordAuthLink = screen.getByText("Email and password");
await userEvent.click(showPasswordAuthLink);

await screen.findByText(Language.passwordSignIn);
await screen.findByText(Language.githubSignIn);
});
});
2 changes: 1 addition & 1 deletion site/src/pages/LoginPage/LoginPageView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ const styles = {

container: {
width: "100%",
maxWidth: 385,
maxWidth: 320,
display: "flex",
flexDirection: "column",
alignItems: "center",
Expand Down
63 changes: 30 additions & 33 deletions site/src/pages/LoginPage/OAuthSignInForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import Link from "@mui/material/Link";
import Button from "@mui/material/Button";
import GitHubIcon from "@mui/icons-material/GitHub";
import KeyIcon from "@mui/icons-material/VpnKey";
Expand Down Expand Up @@ -26,49 +25,47 @@ export const OAuthSignInForm: FC<OAuthSignInFormProps> = ({
return (
<Box display="grid" gap="16px">
{authMethods?.github.enabled && (
<Link
<Button
Copy link
Member

@Parkreiner Parkreiner Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What's the trade-off between having an MUI Button component take an anchor as its underlying component, vs using one of the <a>/<Link> elements directly?

If this is meant to act as a link, I feel like the markup should be more straightforward, but I don't know if this accounting for some edge case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Link works well when doing internal navigation to handle react-router state changes but when it is external, I don't see why to use a Link.

Ps: Link when it is from import { Link } from "react-router-dom".

I use Link from MUI when I want to style it as a link with the blue color and decoration on hover.

component="a"
href={`/api/v2/users/oauth2/github/callback?redirect=${encodeURIComponent(
redirectTo,
)}`}
variant="contained"
startIcon={<GitHubIcon css={iconStyles} />}
disabled={isSigningIn}
fullWidth
type="submit"
size="xlarge"
>
<Button
startIcon={<GitHubIcon css={iconStyles} />}
disabled={isSigningIn}
fullWidth
type="submit"
size="large"
>
{Language.githubSignIn}
</Button>
</Link>
{Language.githubSignIn}
</Button>
)}

{authMethods?.oidc.enabled && (
<Link
<Button
component="a"
href={`/api/v2/users/oidc/callback?redirect=${encodeURIComponent(
redirectTo,
)}`}
variant="contained"
size="xlarge"
startIcon={
authMethods.oidc.iconUrl ? (
<img
alt="Open ID Connect icon"
src={authMethods.oidc.iconUrl}
css={iconStyles}
/>
) : (
<KeyIcon css={iconStyles} />
)
}
disabled={isSigningIn}
fullWidth
type="submit"
>
<Button
size="large"
startIcon={
authMethods.oidc.iconUrl ? (
<img
alt="Open ID Connect icon"
src={authMethods.oidc.iconUrl}
css={iconStyles}
/>
) : (
<KeyIcon css={iconStyles} />
)
}
disabled={isSigningIn}
fullWidth
type="submit"
>
{authMethods.oidc.signInText || Language.oidcSignIn}
</Button>
</Link>
{authMethods.oidc.signInText || Language.oidcSignIn}
</Button>
)}
</Box>
);
Expand Down
48 changes: 22 additions & 26 deletions site/src/pages/LoginPage/PasswordSignInForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,21 @@ import { Stack } from "components/Stack/Stack";
import TextField from "@mui/material/TextField";
import { getFormHelpers, onChangeTrimmed } from "utils/formUtils";
import { Language } from "./SignInForm";
import { FormikContextType, FormikTouched, useFormik } from "formik";
import { useFormik } from "formik";
import * as Yup from "yup";
import { FC } from "react";
import { BuiltInAuthFormValues } from "./SignInForm.types";
import LoadingButton from "@mui/lab/LoadingButton";

type PasswordSignInFormProps = {
onSubmit: (credentials: { email: string; password: string }) => void;
initialTouched?: FormikTouched<BuiltInAuthFormValues>;
isSigningIn: boolean;
autoFocus: boolean;
};

export const PasswordSignInForm: FC<PasswordSignInFormProps> = ({
onSubmit,
initialTouched,
isSigningIn,
autoFocus,
}) => {
const validationSchema = Yup.object({
email: Yup.string()
Expand All @@ -27,25 +26,24 @@ export const PasswordSignInForm: FC<PasswordSignInFormProps> = ({
password: Yup.string(),
});

const form: FormikContextType<BuiltInAuthFormValues> =
useFormik<BuiltInAuthFormValues>({
initialValues: {
email: "",
password: "",
},
validationSchema,
onSubmit,
initialTouched,
});
const getFieldHelpers = getFormHelpers<BuiltInAuthFormValues>(form);
const form = useFormik({
initialValues: {
email: "",
password: "",
},
validationSchema,
onSubmit,
validateOnBlur: false,
});
const getFieldHelpers = getFormHelpers(form);

return (
<form onSubmit={form.handleSubmit}>
<Stack spacing={2.5}>
<TextField
{...getFieldHelpers("email")}
onChange={onChangeTrimmed(form)}
autoFocus
autoFocus={autoFocus}
autoComplete="email"
fullWidth
label={Language.emailLabel}
Expand All @@ -59,16 +57,14 @@ export const PasswordSignInForm: FC<PasswordSignInFormProps> = ({
label={Language.passwordLabel}
type="password"
/>
<div>
<LoadingButton
size="large"
loading={isSigningIn}
fullWidth
type="submit"
>
{Language.passwordSignIn}
</LoadingButton>
</div>
<LoadingButton
size="xlarge"
loading={isSigningIn}
fullWidth
type="submit"
>
{Language.passwordSignIn}
</LoadingButton>
</Stack>
</form>
);
Expand Down
3 changes: 0 additions & 3 deletions site/src/pages/LoginPage/SignInForm.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,6 @@ export const WithError: Story = {
},
],
}),
initialTouched: {
password: true,
},
},
};

Expand Down
54 changes: 11 additions & 43 deletions site/src/pages/LoginPage/SignInForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,10 @@
import { type Interpolation, type Theme } from "@emotion/react";
import { type FormikTouched } from "formik";
import { type FC, useState } from "react";
import { type FC } from "react";
import type { AuthMethods } from "api/typesGenerated";
import { PasswordSignInForm } from "./PasswordSignInForm";
import { OAuthSignInForm } from "./OAuthSignInForm";
import { type BuiltInAuthFormValues } from "./SignInForm.types";
import Button from "@mui/material/Button";
import EmailIcon from "@mui/icons-material/EmailOutlined";
import { Alert } from "components/Alert/Alert";
import { ErrorAlert } from "components/Alert/ErrorAlert";
import { getApplicationName } from "utils/appearance";

export const Language = {
emailLabel: "Email",
Expand Down Expand Up @@ -71,8 +66,6 @@ export interface SignInFormProps {
info?: string;
authMethods?: AuthMethods;
onSubmit: (credentials: { email: string; password: string }) => void;
// initialTouched is only used for testing the error state of the form.
initialTouched?: FormikTouched<BuiltInAuthFormValues>;
}

export const SignInForm: FC<React.PropsWithChildren<SignInFormProps>> = ({
Expand All @@ -82,21 +75,15 @@ export const SignInForm: FC<React.PropsWithChildren<SignInFormProps>> = ({
error,
info,
onSubmit,
initialTouched,
}) => {
const oAuthEnabled = Boolean(
authMethods?.github.enabled || authMethods?.oidc.enabled,
);
const passwordEnabled = authMethods?.password.enabled ?? true;
// Hide password auth by default if any OAuth method is enabled
const [showPasswordAuth, setShowPasswordAuth] = useState(!oAuthEnabled);
const applicationName = getApplicationName();

return (
<div css={styles.root}>
<h1 css={styles.title}>
Sign in to <strong>{applicationName}</strong>
</h1>
<h1 css={styles.title}>Sign in</h1>

{Boolean(error) && (
<div css={styles.alert}>
Expand All @@ -110,52 +97,33 @@ export const SignInForm: FC<React.PropsWithChildren<SignInFormProps>> = ({
</div>
)}

{passwordEnabled && showPasswordAuth && (
<PasswordSignInForm
onSubmit={onSubmit}
initialTouched={initialTouched}
{oAuthEnabled && (
<OAuthSignInForm
isSigningIn={isSigningIn}
redirectTo={redirectTo}
authMethods={authMethods}
/>
)}

{passwordEnabled && showPasswordAuth && oAuthEnabled && (
{passwordEnabled && oAuthEnabled && (
<div css={styles.divider}>
<div css={styles.dividerLine} />
<div css={styles.dividerLabel}>Or</div>
<div css={styles.dividerLine} />
</div>
)}

{oAuthEnabled && (
<OAuthSignInForm
{passwordEnabled && (
<PasswordSignInForm
onSubmit={onSubmit}
autoFocus={!oAuthEnabled}
isSigningIn={isSigningIn}
redirectTo={redirectTo}
authMethods={authMethods}
/>
)}

{!passwordEnabled && !oAuthEnabled && (
<Alert severity="error">No authentication methods configured!</Alert>
)}

{passwordEnabled && !showPasswordAuth && (
<>
<div css={styles.divider}>
<div css={styles.dividerLine} />
<div css={styles.dividerLabel}>Or</div>
<div css={styles.dividerLine} />
</div>

<Button
fullWidth
size="large"
onClick={() => setShowPasswordAuth(true)}
startIcon={<EmailIcon css={styles.icon} />}
>
Email and password
</Button>
</>
)}
</div>
);
};
9 changes: 0 additions & 9 deletions site/src/pages/LoginPage/SignInForm.types.ts
View file
Open in desktop

This file was deleted.

1 change: 1 addition & 0 deletions site/src/theme/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export const sidePadding = 24;
export const dashboardContentBottomPadding = 8 * 6;

// MUI does not have aligned heights for buttons and inputs so we have to "hack" it a little bit
export const BUTTON_XL_HEIGHT = 44;
export const BUTTON_LG_HEIGHT = 40;
export const BUTTON_MD_HEIGHT = 36;
export const BUTTON_SM_HEIGHT = 32;
Loading