- Notifications
You must be signed in to change notification settings - Fork928
refactor(site): SignInForm without wrapper component#558
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes from4 commits
b3bdf83d2741c3918dd8073df2dae1e12d8108c77e50e6e0c17bc113c5619f8File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,25 @@ | ||
| import { FormikContextType } from "formik/dist/types" | ||
| export * from "./FormCloseButton" | ||
| export * from "./FormSection" | ||
| export * from "./FormDropdownField" | ||
| export * from "./FormTextField" | ||
| export * from "./FormTitle" | ||
| export function getFormHelpers<T>(form: FormikContextType<T>, name: keyof T) { | ||
| const touched = form.touched[name] | ||
| const errors = form.errors[name] | ||
| return { | ||
| ...form.getFieldProps(name), | ||
| id: name, | ||
| error: touched && Boolean(errors), | ||
| helperText: touched && errors | ||
| } | ||
| } | ||
| export function onChangeTrimmed<T>(form: FormikContextType<T>) { | ||
| return (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| event.target.value = event?.target?.value?.trim() | ||
| form.handleChange(event) | ||
| } | ||
| } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. praise: nice, this approach seems great to me. Question on code conventions --> are we going to adopt using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I've always preferred arrow functions but I don't know if there's a good reason to anymore. I think you just have to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. These can be safely ported over to arrow fns: constgenericFn=<T,>()=>{// impl} Contributor
| ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -4,9 +4,10 @@ import React from "react" | ||
| import * as Yup from "yup" | ||
| import { Welcome } from "./Welcome" | ||
| import FormHelperText from "@material-ui/core/FormHelperText" | ||
| import { LoadingButton } from "./../Button" | ||
| import TextField from "@material-ui/core/TextField" | ||
| import { getFormHelpers, onChangeTrimmed } from "../Form" | ||
| /** | ||
| * BuiltInAuthFormValues describes a form using built-in (email/password) | ||
| @@ -18,8 +19,17 @@ interface BuiltInAuthFormValues { | ||
| password: string | ||
| } | ||
| export const LANGUAGE = { | ||
presleyp marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| emailLabel: "Email", | ||
| passwordLabel: "Password", | ||
| emailInvalid: "Please enter a valid email address.", | ||
| emailRequired: "Please enter an email address.", | ||
| authErrorMessage: "Incorrect email or password.", | ||
| signIn: "Sign In", | ||
| } | ||
| const validationSchema = Yup.object({ | ||
| email: Yup.string().trim().email(LANGUAGE.emailInvalid).required(LANGUAGE.emailRequired), | ||
| password: Yup.string(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @vapurrmaid@kylecarbs do we want validation on the password? Contributor
| ||
| }) | ||
| @@ -59,50 +69,34 @@ export const SignInForm: React.FC<SignInFormProps> = ({ isLoading, authErrorMess | ||
| <> | ||
| <Welcome /> | ||
| <form onSubmit={form.handleSubmit}> | ||
| <TextField | ||
| {...getFormHelpers<BuiltInAuthFormValues>(form, "email")} | ||
| onChange={onChangeTrimmed(form)} | ||
| autoFocus | ||
| autoComplete="email" | ||
| className={styles.loginTextField} | ||
| fullWidth | ||
| label={LANGUAGE.emailLabel} | ||
| variant="outlined" | ||
| /> | ||
| <TextField | ||
| {...getFormHelpers<BuiltInAuthFormValues>(form, "password")} | ||
| autoComplete="current-password" | ||
| className={styles.loginTextField} | ||
| fullWidth | ||
| id="password" | ||
| label={LANGUAGE.passwordLabel} | ||
| type="password" | ||
| variant="outlined" | ||
| /> | ||
| {authErrorMessage && ( | ||
| <FormHelperText data-testid="sign-in-error" error> | ||
| {LANGUAGE.authErrorMessage} | ||
| </FormHelperText> | ||
| )} | ||
| <div className={styles.submitBtn}> | ||
| <LoadingButton color="primary" loading={isLoading} fullWidth type="submit" variant="contained"> | ||
| {isLoading ? "" : LANGUAGE.signIn} | ||
| </LoadingButton> | ||
| </div> | ||
| </form> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,11 @@ | ||
| import React from "react" | ||
| import { act, screen } from "@testing-library/react" | ||
| import userEvent from "@testing-library/user-event" | ||
| import { history, render } from "../test_helpers" | ||
| import { SignInPage } from "./login" | ||
| import { server } from "../test_helpers/server" | ||
| import { rest } from "msw" | ||
| import { LANGUAGE } from "../components/SignIn/SignInForm" | ||
| describe("SignInPage", () => { | ||
| beforeEach(() => { | ||
| @@ -21,12 +23,12 @@ describe("SignInPage", () => { | ||
| render(<SignInPage />) | ||
| // Then | ||
| await screen.findByText(LANGUAGE.signIn, { exact: false }) | ||
| }) | ||
| it("shows an error message if SignIn fails", async () => { | ||
| // Given | ||
| render(<SignInPage />) | ||
| // Make login fail | ||
| server.use( | ||
| rest.post("/api/v2/users/login", async (req, res, ctx) => { | ||
| @@ -35,17 +37,18 @@ describe("SignInPage", () => { | ||
| ) | ||
| // When | ||
| // Set email / password | ||
| const email = screen.getByLabelText(LANGUAGE.emailLabel) | ||
| const password = screen.getByLabelText(LANGUAGE.passwordLabel) | ||
| userEvent.type(email, "test@coder.com") | ||
| userEvent.type(password, "password") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. praise: This is looking really nice! | ||
| // Click sign-in | ||
| const signInButton = await screen.findByText(LANGUAGE.signIn) | ||
| act(() => signInButton.click()) | ||
| // Then | ||
| // Finding error by test id because it comes from the backend | ||
| const errorMessage = await screen.findByText(LANGUAGE.authErrorMessage) | ||
| expect(errorMessage).toBeDefined() | ||
| expect(history.location.pathname).toEqual("/login") | ||
| }) | ||