Skip to content

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.

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 9 commits into from
Mar 30, 2022
Merged

refactor(site): SignInForm without wrapper component #558

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b3bdf83
Remove wrapper from SignInForm
presleyp Mar 24, 2022
d2741c3
Spruce up tests
presleyp Mar 24, 2022
918dd80
Add util for form props
presleyp Mar 24, 2022
73df2da
Add back trim
presleyp Mar 24, 2022
e1e12d8
Add unit tests
presleyp Mar 24, 2022
108c77e
Lint, type fixes
presleyp Mar 24, 2022
50e6e0c
Pascal case for language
presleyp Mar 28, 2022
17bc113
Arrow functions
presleyp Mar 28, 2022
c5619f8
Target text in e2e
presleyp Mar 30, 2022
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
Next Next commit
Remove wrapper from SignInForm
  • Loading branch information
@presleyp
presleyp committed Mar 24, 2022
commit b3bdf83b2771918d9a7c898b83a37c7ab51ce11c
88 changes: 40 additions & 48 deletions site/src/components/SignIn/SignInForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ 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"

/**
* BuiltInAuthFormValues describes a form using built-in (email/password)
Expand All @@ -18,8 +18,14 @@ interface BuiltInAuthFormValues {
password: string
}

export const LANGUAGE = {
emailInvalid: "Please enter a valid email address.",
emailRequired: "Please enter an email address.",
authErrorMessage: "Incorrect email or password."
}

const validationSchema = Yup.object({
email: Yup.string().required("Email is required."),
email: Yup.string().trim().email(LANGUAGE.emailInvalid).required(LANGUAGE.emailRequired),
password: Yup.string(),
Copy link
Contributor Author

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

@greyscaled greyscaled Mar 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.

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

Copy link
Contributor Author

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.

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 OpenAPI pattern 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 like interdependent fields though.
  • Idea 2: We could ensure that the backend accepts a parameter validate (or whatever you want to name it) that will signal the backend to only perform field validations and no other action.
    • Note: we won't be able to validate everything, 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.

})

Expand Down Expand Up @@ -59,52 +65,38 @@ 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>
<div className={styles.submitBtn}>
<LoadingButton
color="primary"
loading={isLoading}
fullWidth
id="signin-form-submit"
type="submit"
variant="contained"
>
{isLoading ? "" : "Sign In"}
</LoadingButton>
</div>
<TextField
{...form.getFieldProps("email")}
autoFocus
autoComplete="email"
className={styles.loginTextField}
error={form.touched.email && Boolean(form.errors.email)}
fullWidth
helperText={form.touched.email && form.errors.email}
id="email"
label="Email"
variant="outlined"
/>
<TextField
{...form.getFieldProps("password")}
autoComplete="current-password"
className={styles.loginTextField}
fullWidth
id="password"
label="Password"
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 ? "" : "Sign In"}
</LoadingButton>
</div>
</form>
</>
)
Expand Down