Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

Merged
presleyp merged 9 commits intomainfrom451/presleyp/forms
Mar 30, 2022
Merged
Show file tree
Hide file tree
Changes fromall commits
Commits
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
6 changes: 3 additions & 3 deletionssite/e2e/pom/SignInPage.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,8 +7,8 @@ export class SignInPage extends BasePom {
}

async submitBuiltInAuthentication(email: string, password: string): Promise<void> {
await this.page.fill("id=signin-form-inpt-email", email)
await this.page.fill("id=signin-form-inpt-password", password)
await this.page.click("id=signin-form-submit")
await this.page.fill("text=Email", email)
await this.page.fill("text=Password", password)
await this.page.click("text=Sign In")
}
}
1 change: 1 addition & 0 deletionssite/package.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -48,6 +48,7 @@
"@storybook/addon-links": "6.4.19",
"@storybook/react": "6.4.19",
"@testing-library/react": "12.1.4",
"@testing-library/user-event": "^13.5.0",
"@types/express": "4.17.13",
"@types/jest": "27.4.1",
"@types/node": "14.18.12",
Expand Down
74 changes: 74 additions & 0 deletionssite/src/components/Form/index.test.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
import { FormikContextType } from "formik/dist/types"
import { getFormHelpers, onChangeTrimmed } from "./index"

interface TestType {
untouchedGoodField: string
untouchedBadField: string
touchedGoodField: string
touchedBadField: string
}

const mockHandleChange = jest.fn()

const form = {
errors: {
untouchedGoodField: undefined,
untouchedBadField: "oops!",
touchedGoodField: undefined,
touchedBadField: "oops!",
},
touched: {
untouchedGoodField: false,
untouchedBadField: false,
touchedGoodField: true,
touchedBadField: true,
},
handleChange: mockHandleChange,
handleBlur: jest.fn(),
getFieldProps: (name: string) => {
return {
name,
onBlur: jest.fn(),
onChange: jest.fn(),
value: "",
}
},
} as unknown as FormikContextType<TestType>

describe("form util functions", () => {
describe("getFormHelpers", () => {
const untouchedGoodResult = getFormHelpers<TestType>(form, "untouchedGoodField")
const untouchedBadResult = getFormHelpers<TestType>(form, "untouchedBadField")
const touchedGoodResult = getFormHelpers<TestType>(form, "touchedGoodField")
const touchedBadResult = getFormHelpers<TestType>(form, "touchedBadField")
it("populates the 'field props'", () => {
expect(untouchedGoodResult.name).toEqual("untouchedGoodField")
expect(untouchedGoodResult.onBlur).toBeDefined()
expect(untouchedGoodResult.onChange).toBeDefined()
expect(untouchedGoodResult.value).toBeDefined()
})
it("sets the id to the name", () => {
expect(untouchedGoodResult.id).toEqual("untouchedGoodField")
})
it("sets error to true if touched and invalid", () => {
expect(untouchedGoodResult.error).toBeFalsy
expect(untouchedBadResult.error).toBeFalsy
expect(touchedGoodResult.error).toBeFalsy
expect(touchedBadResult.error).toBeTruthy
})
it("sets helperText to the error message if touched and invalid", () => {
expect(untouchedGoodResult.helperText).toBeUndefined
expect(untouchedBadResult.helperText).toBeUndefined
expect(touchedGoodResult.helperText).toBeUndefined
expect(touchedBadResult.helperText).toEqual("oops!")
})
})

describe("onChangeTrimmed", () => {
it("calls handleChange with trimmed value", () => {
const event = { target: { value: " hello " } } as React.ChangeEvent<HTMLInputElement>
onChangeTrimmed<TestType>(form)(event)
expect(mockHandleChange).toHaveBeenCalledWith({ target: { value: "hello" } })
})
})
})
32 changes: 32 additions & 0 deletionssite/src/components/Form/index.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,37 @@
import { FormikContextType, getIn } from "formik"
import { ChangeEvent, ChangeEventHandler, FocusEventHandler } from "react"

export * from "./FormCloseButton"
export * from "./FormSection"
export * from "./FormDropdownField"
export * from "./FormTextField"
export * from "./FormTitle"

interface FormHelpers {
name: string
onBlur: FocusEventHandler
onChange: ChangeEventHandler
id: string
value?: string | number
error: boolean
helperText?: string
}

export const getFormHelpers = <T>(form: FormikContextType<T>, name: string): FormHelpers => {
// getIn is a util function from Formik that gets at any depth of nesting, and is necessary for the types to work
const touched = getIn(form.touched, name)
const errors = getIn(form.errors, name)
return {
...form.getFieldProps(name),
id: name,
error: touched && Boolean(errors),
helperText: touched && errors,
}
}

export const onChangeTrimmed =
<T>(form: FormikContextType<T>) =>
(event: ChangeEvent<HTMLInputElement>): void => {
event.target.value = event.target.value.trim()
form.handleChange(event)
}
80 changes: 35 additions & 45 deletionssite/src/components/SignIn/SignInForm.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -4,9 +4,10 @@ import React from "react"
import * as Yup from "yup"

import { Welcome } from "./Welcome"
import { FormTextField } from "../Form"
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)
Expand All@@ -18,8 +19,17 @@ interface BuiltInAuthFormValues {
password: string
}

export const Language = {
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().required("Email isrequired."),
email: Yup.string().trim().email(Language.emailInvalid).required(Language.emailRequired),
password: Yup.string(),
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@vapurrmaid@kylecarbs do we want validation on the password?

Copy link
Contributor

@greyscaledgreyscaledMar 24, 2022•
edited
Loading

Choose a reason for hiding this comment

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

This is the same as it is in V1, I'm inclined to circle back to it later since it's separate from this change and would need some tests. I think that overhead warrants it as a separate concern.

There's also potentially the desire to have the validation regex come from the backend so that it's centralized. We can brainstorm more on that over gap week next week.

cc@f0ssel@deansheather@johnstcn@coadler --> we've all spoken about centralizing things in the backend at some point in a recent breakout session, would love to noodle on moving validation/regexes into the BE as well, to see if it's a worthwhile idea to explore.

presleyp and kylecarbs reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

I think in this case if we just make it so the backend does the validation and then returns a per-field error that the frontend can parse then it's the same effect without having to do weird stuff like returning regexes from the backend

johnstcn and greyscaled reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Worth considering, but we should also think about the user experience of having to submit your form before you find out that it's invalid.

greyscaled reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

Worth considering, but we should also think about the user experience of having to submit your form before you find out that it's invalid.

  • Idea 1: We could just leverage the OpenAPIpattern field for stuff like regexes and use auto-generation magic to keep validations in sync across FE and BE. This wouldn't let us do fancy stuff likeinterdependent fields though.
  • Idea 2: We could ensure that the backend accepts a parametervalidate (or whatever you want to name it) that will signal the backend toonly perform field validations and no other action.
    • Note: we won't be able to validateeverything, and this also incurs additional request overhead as I'd imagine we'd want to trigger a fresh validation request on every change in the form.

greyscaled reacted with thumbs up emoji
})

Expand DownExpand Up@@ -59,50 +69,30 @@ export const SignInForm: React.FC<SignInFormProps> = ({ isLoading, authErrorMess
<>
<Welcome />
<form onSubmit={form.handleSubmit}>
<div>
<FormTextField
label="Email"
autoComplete="email"
autoFocus
className={styles.loginTextField}
eventTransform={(email: string) => email.trim()}
form={form}
formFieldName="email"
fullWidth
inputProps={{
id: "signin-form-inpt-email",
}}
variant="outlined"
/>
<FormTextField
label="Password"
autoComplete="current-password"
className={styles.loginTextField}
form={form}
formFieldName="password"
fullWidth
inputProps={{
id: "signin-form-inpt-password",
}}
isPassword
variant="outlined"
/>
{authErrorMessage && (
<FormHelperText data-testid="sign-in-error" error>
{authErrorMessage}
</FormHelperText>
)}
</div>
<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 error>{Language.authErrorMessage}</FormHelperText>}
<div className={styles.submitBtn}>
<LoadingButton
color="primary"
loading={isLoading}
fullWidth
id="signin-form-submit"
type="submit"
variant="contained"
>
{isLoading ? "" : "Sign In"}
<LoadingButton color="primary" loading={isLoading} fullWidth type="submit" variant="contained">
{isLoading ? "" : Language.signIn}
</LoadingButton>
</div>
</form>
Expand Down
21 changes: 12 additions & 9 deletionssite/src/pages/login.test.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
import React from "react"
import { act, fireEvent, screen } from "@testing-library/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(() => {
Expand All@@ -21,12 +23,12 @@ describe("SignInPage", () => {
render(<SignInPage />)

// Then
await screen.findByText("Sign In", { exact: false })
await screen.findByText(Language.signIn, { exact: false })
})

it("shows an error message if SignIn fails", async () => {
// Given
const { container } =render(<SignInPage />)
render(<SignInPage />)
// Make login fail
server.use(
rest.post("/api/v2/users/login", async (req, res, ctx) => {
Expand All@@ -35,17 +37,18 @@ describe("SignInPage", () => {
)

// When
// Set username / password
const [username, password] = container.querySelectorAll("input")
fireEvent.change(username, { target: { value: "test@coder.com" } })
fireEvent.change(password, { target: { value: "password" } })
// 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")
Copy link
Contributor

Choose a reason for hiding this comment

The 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("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.findByTestId("sign-in-error")
const errorMessage = await screen.findByText(Language.authErrorMessage)
expect(errorMessage).toBeDefined()
expect(history.location.pathname).toEqual("/login")
})
Expand Down
7 changes: 7 additions & 0 deletionssite/yarn.lock
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2742,6 +2742,13 @@
"@testing-library/dom" "^8.0.0"
"@types/react-dom" "*"

"@testing-library/user-event@^13.5.0":
version "13.5.0"
resolved "https://registry.yarnpkg.com/@testing-library/user-event/-/user-event-13.5.0.tgz#69d77007f1e124d55314a2b73fd204b333b13295"
integrity sha512-5Kwtbo3Y/NowpkbRuSepbyMFkZmHgD+vPzYB/RJ4oxt5Gj/avFFBYjhw27cqSVPVw/3a67NK1PbiIr9k4Gwmdg==
dependencies:
"@babel/runtime" "^7.12.5"

"@tootallnate/once@1":
version "1.1.2"
resolved "https://registry.yarnpkg.com/@tootallnate/once/-/once-1.1.2.tgz#ccb91445360179a04e7fe6aff78c00ffc1eeaf82"
Expand Down
[8]ページ先頭
©2009-2025 Movatter.jp