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

Show file tree
Hide file tree
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
Prev Previous commit
Next Next commit
Lint, type fixes
  • Loading branch information
@presleyp
presleyp committed Mar 24, 2022
commit 108c77e727127e1b201301f79c51fedf49b12f65
14 changes: 7 additions & 7 deletions site/src/components/Form/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { FormikContextType } from 'formik/dist/types'
import { getFormHelpers, onChangeTrimmed } from './index'
import { FormikContextType } from "formik/dist/types"
import { getFormHelpers, onChangeTrimmed } from "./index"

interface TestType {
untouchedGoodField: string
Expand All @@ -10,12 +10,12 @@ interface TestType {

const mockHandleChange = jest.fn()

const form = {
const form = {
errors: {
untouchedGoodField: undefined,
untouchedBadField: "oops!",
touchedGoodField: undefined,
touchedBadField: 'oops!',
touchedBadField: "oops!",
},
touched: {
untouchedGoodField: false,
Expand All @@ -30,9 +30,9 @@ const form = {
name,
onBlur: jest.fn(),
onChange: jest.fn(),
value: ''
value: "",
}
}
},
} as unknown as FormikContextType<TestType>

describe("form util functions", () => {
Expand Down Expand Up @@ -66,7 +66,7 @@ describe("form util functions", () => {

describe("onChangeTrimmed", () => {
it("calls handleChange with trimmed value", () => {
const event = { target: { value: " hello "}} as React.ChangeEvent<HTMLInputElement>
const event = { target: { value: " hello " } } as React.ChangeEvent<HTMLInputElement>
onChangeTrimmed<TestType>(form)(event)
expect(mockHandleChange).toHaveBeenCalledWith({ target: { value: "hello" } })
})
Expand Down
38 changes: 25 additions & 13 deletions site/src/components/Form/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,37 @@
import { FormikContextType } from "formik/dist/types"
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"

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
}
interface FormHelpers {
name: string
onBlur: FocusEventHandler
onChange: ChangeEventHandler
id: string
value?: string | number
error: boolean
helperText?: string
}

export function 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 function onChangeTrimmed<T>(form: FormikContextType<T>) {
return (event: React.ChangeEvent<HTMLInputElement>) => {
event.target.value = event?.target?.value?.trim()
export function onChangeTrimmed<T>(form: FormikContextType<T>): (event: ChangeEvent<HTMLInputElement>) => void {
return (event: ChangeEvent<HTMLInputElement>): void => {
event.target.value = event.target.value.trim()
form.handleChange(event)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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 function instead of const arrow fns for pure functions in v2? Stylistically, it's my preference too. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 function for generics. What do you prefer about them?

Copy link
Contributor

Choose a reason for hiding this comment

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

These can be safely ported over to arrow fns:

const genericFn = <T,>() => {
  // impl
}

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.

What do you prefer about them?

Simply the style: when all fns look the same, my brain is happy. I dislike missing braces/early returns and having to scan the difference between a non-fn const and a fn const.

Visualized, I mean:

// my brain is happy with this

const myVar = "myVar"

function thing() {
  return "thing"
}

[1, 2, 3].map((num) => {
  return num + 1
})

if (myVar) {
  thing()
}
// my brain is not happy with this

const myVar = "myVar"

const thing = () => "thing"

[1, 2, 3].map(num => num++)

if (myVar) thing()

My brain has to do extra processing to understand version 2, even though it's shorter in length. I think this is due to how much I can "read between the lines". In version one, things are blocked in a way that's easy for me to interpret in the background; ultimately I think I "absorb more information per eye scan" or something like that. In the second version, I often find myself double-passing/re-reading some blocks to understand the big picture.

I won't die on any of these hills, because my brain and its preferences don't get to dictate these things; we make those decisions together as a team. At the end of the day, consistency is best to shoot for, and we already use the arrow fns, so I think we should convert these over to be arrow fns.

The only time we should reach for function is for special cases around scoping this inside the function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh thanks, didn't know the trick for making the generic work with an arrow function. But yeah I see your point! I'm down to switch to function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to put this issue on the list of things to discuss at a future FE V and stick to arrows for now.