Skip to content
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

feat(js): update Formik to v2 #5190

Merged
merged 7 commits into from
Mar 11, 2020
Merged

feat(js): update Formik to v2 #5190

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4db2696
feat(js): update Formik to v2
IanLondon Mar 9, 2020
d7a0035
remove old formik type defs, fix flow errors
IanLondon Mar 9, 2020
3e15f13
fix labware creator field components
IanLondon Mar 10, 2020
e2c94b1
fix ConfigurePipette form
IanLondon Mar 10, 2020
ba301ec
fix ConnectForm regression
IanLondon Mar 10, 2020
b976368
fix LiquidPlacementForm regression
IanLondon Mar 10, 2020
bb55ace
revert SelectField, address PR comments
IanLondon Mar 10, 2020
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
2 changes: 1 addition & 1 deletion app/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"core-js": "^3.2.1",
"date-fns": "^2.10.0",
"events": "^3.0.0",
"formik": "^1.3.1",
"formik": "^2.1.4",
"history": "^4.7.2",
"lodash": "^4.17.4",
"mixpanel-browser": "^2.22.1",
Expand Down
8 changes: 6 additions & 2 deletions app/src/components/AppSettings/AddManualIp/ManualIpForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { startDiscovery } from '../../../discovery'
import { Formik, Form, Field } from 'formik'
import { IpField } from './IpField'

import type { FormikProps } from 'formik/@flow-typed'
import type { State, Dispatch } from '../../../types'
import type { DiscoveryCandidates } from '../../../config/types'

Expand All @@ -18,6 +19,8 @@ type DP = {| addManualIp: (ip: string) => mixed |}

type Props = {| ...SP, ...DP |}

type ManualIpFormValues = {| ip: string |}

class ManualIpFormComponent extends React.Component<Props> {
inputRef: { current: null | HTMLInputElement }

Expand All @@ -43,14 +46,15 @@ class ManualIpFormComponent extends React.Component<Props> {

actions.resetForm()
}}
render={formProps => {
>
{(formProps: FormikProps<ManualIpFormValues>) => {
mcous marked this conversation as resolved.
Show resolved Hide resolved
return (
<Form>
<Field name="ip" component={IpField} inputRef={this.inputRef} />
</Form>
)
}}
/>
</Formik>
)
}
}
Expand Down
26 changes: 15 additions & 11 deletions app/src/components/ConfigurePipette/ConfigForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
ConfigQuirkGroup,
} from './ConfigFormGroup'

import type { FormikProps } from 'formik/@flow-typed'
import type {
PipetteSettingsField,
PipetteSettingsFieldsMap,
Expand Down Expand Up @@ -195,18 +196,21 @@ export class ConfigForm extends React.Component<ConfigFormProps> {
initialValues={initialValues}
validate={this.validate}
validateOnChange={false}
render={formProps => {
>
{(formProps: FormikProps<FormValues>) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just changed from render prop to child fn

const { errors, values } = formProps
const disableSubmit = !isEmpty(errors)
const handleReset = () =>
formProps.resetForm(
mapValues(values, v => {
if (typeof v === 'boolean') {
return true
}
return ''
})
)
const handleReset = () => {
const newValues = mapValues(values, v => {
if (typeof v === 'boolean') {
// NOTE: checkbox fields don't have defaults from the API b/c they come in from `quirks`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this isn't a change, I just added a note b/c I got confused about what was intended here 🙃

// For now, we'll reset all checkboxes to true
return true
}
return ''
})
formProps.resetForm({ values: newValues })
}
return (
<Form>
<FormColumn>
Expand Down Expand Up @@ -264,7 +268,7 @@ export class ConfigForm extends React.Component<ConfigFormProps> {
</Form>
)
}}
/>
</Formik>
)
}
}
22 changes: 12 additions & 10 deletions app/src/components/ConfigurePipette/ConfigFormGroup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// @flow
import * as React from 'react'
import { Field } from 'formik'
import { Field, type FieldProps } from 'formik'
import { FormGroup, InputField, CheckboxField } from '@opentrons/components'

import styles from './styles.css'
Expand Down Expand Up @@ -79,15 +79,17 @@ export function ConfigInput(props: ConfigInputProps) {
return (
<ConfigFormRow label={displayName} labelFor={id}>
<Field name={name}>
{fieldProps => (
{(fieldProps: FieldProps<DisplayFieldProps>) => (
<InputField
placeholder={_default}
name={fieldProps.field.name}
value={String(fieldProps.field.value ?? '')}
onChange={fieldProps.field.onChange}
onBlur={fieldProps.field.onBlur}
error={fieldProps.form.errors[name]}
{...{
...fieldProps.field,
units,
className,
error: fieldProps.form.errors[name],
touched: fieldProps.form.touched[name],
}}
/>
)}
Expand All @@ -107,13 +109,13 @@ export function ConfigCheckbox(props: ConfigCheckboxProps) {
const id = makeId(name)
return (
<ConfigFormRow label={displayName} labelFor={id}>
<Field name={name}>
<Field name={name} type="checkbox">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

type="checkbox" is required for fieldProps.field.checked to work

{fieldProps => (
<CheckboxField
{...{
...fieldProps.field,
className,
}}
name={fieldProps.field.name}
onChange={fieldProps.field.onChange}
value={fieldProps.field.checked}
className={className}
/>
)}
</Field>
Expand Down
34 changes: 22 additions & 12 deletions app/src/components/RobotSettings/SelectNetwork/ConnectForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import type {
} from '../../../http-api-client'

import type { SelectOptionOrGroup } from '@opentrons/components'
import type { FormikProps } from 'formik/@flow-typed'

type ConnectFormProps = {|
ssid: ?string,
Expand Down Expand Up @@ -223,20 +224,28 @@ export class ConnectForm extends React.Component<
return get(values, name)
}

handleSecurityChange(
handleSecurityChange(args: {|
name: string,
value: ?string,
ssid: ?string,
setValues: FormValues => mixed
): mixed {
setValues: *,
mcous marked this conversation as resolved.
Show resolved Hide resolved
resetForm: *,
|}): mixed {
const { name, value, ssid, setValues, resetForm } = args
const { eapOptions, securityType: knownSecurityType } = this.props
const nextValues = ssid ? { ssid } : {}
const eapType = find(eapOptions, { name: value })
const securityValue = eapType ? WPA_EAP_SECURITY : value

if (!knownSecurityType) set(nextValues, name, securityValue)
if (eapType) set(nextValues, EAP_TYPE_FIELD, value)
setValues(nextValues)
// resetting the form on NO_SECURITY clears
// validation, disabling submit, so use setValues
if (value === NO_SECURITY) {
setValues(nextValues)
} else {
resetForm({ values: nextValues })
}
}

render() {
Expand All @@ -247,7 +256,9 @@ export class ConnectForm extends React.Component<
<Formik
onSubmit={this.handleSubmit}
validate={this.validate}
render={formProps => {
initialValues={{}}
>
{(formProps: FormikProps<FormValues>) => {
const {
values,
errors,
Expand Down Expand Up @@ -301,15 +312,14 @@ export class ConnectForm extends React.Component<
placeholder={SECURITY_TYPE_PLACEHOLDER}
onLoseFocus={setFieldTouched}
onValueChange={(name, value) => {
this.handleSecurityChange(
this.handleSecurityChange({
name,
value,
// needed to ensure SSID is not wiped out
this.getSsid(values),
// resetting the form on NO_SECURITY clears
// validation, disabling submit, so use setValues
value === NO_SECURITY ? setValues : resetForm
)
ssid: this.getSsid(values),
setValues,
resetForm,
})
}}
/>
)
Expand Down Expand Up @@ -345,7 +355,7 @@ export class ConnectForm extends React.Component<
</form>
)
}}
/>
</Formik>
)
}
}
19 changes: 16 additions & 3 deletions components/src/forms/SelectField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// @flow
import * as React from 'react'
import React, { useMemo, type Node } from 'react'
import assert from 'assert'
import cx from 'classnames'
import find from 'lodash/find'

Expand Down Expand Up @@ -28,13 +29,14 @@ export type SelectFieldProps = {|
/** optional className */
className?: string,
/** optional caption. hidden when `error` is given */
caption?: React.Node,
caption?: Node,
/** if included, use error style and display error instead of caption */
error?: ?string,
/** change handler called with (name, value) */
onValueChange?: (name: string, value: string) => mixed,
/** blur handler called with (name) */
onLoseFocus?: (name: string) => mixed,
onBlur?: (e: SyntheticFocusEvent<HTMLElement>) => mixed,
|}

export function SelectField(props: SelectFieldProps) {
Expand All @@ -50,13 +52,24 @@ export function SelectField(props: SelectFieldProps) {
error,
onValueChange,
onLoseFocus,
onBlur,
} = props
const allOptions = options.flatMap(og => og.options || [og])
const value = find(allOptions, { value: props.value }) || null
const caption = error || props.caption
const captionCx = cx(styles.select_caption, { [styles.error]: error })
const fieldCx = cx(styles.select_field, { [styles.error]: error }, className)

if (props.onLoseFocus && props.onBlur) {
assert(
false,
'SelectField should use `onLoseFocus` OR `onBlur` prop, not both'
)
}
const handleOnBlur = useMemo(() => {
return onLoseFocus ? () => onLoseFocus(name) : onBlur
}, [onLoseFocus, onBlur, name])
mcous marked this conversation as resolved.
Show resolved Hide resolved

return (
<div>
<Select
Expand All @@ -70,7 +83,7 @@ export function SelectField(props: SelectFieldProps) {
menuPosition={menuPosition}
formatOptionLabel={formatOptionLabel}
onChange={opt => onValueChange && onValueChange(name, opt?.value || '')}
onBlur={() => onLoseFocus && onLoseFocus(name)}
onBlur={handleOnBlur}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcous do you know why we didn't do something like this earlier? Formik needs that event to handle onBlur, otherwise it will throw an error at runtime (eg in labware-library/src/labware-creator/components/Dropdown.js)

Copy link
Contributor

@mcous mcous Mar 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

SelectField is an old component, and was always designed to use setFieldValue and setFieldTouched rather than onChange and onBlur (see SelectField.md). The reasons behind this were:

  • onBlur is attached to the <input type="text"> that <ReactSelect> sometimes (maybe? It's pretty hard to tell from the react-select docs so this is a combo of code reading and docs inference) renders depending on what props you pass in
  • onChange in <ReactSelect> is not "standard", so it felt weird to have a "vanila" onBlur but a custom onChange

Accordingly, it looks lile labware-library/src/labware-creator/components/Dropdown.js was written incorrectly for how SelectField is designed to be used:

// is now
onLoseFocus={() => {
  reportFieldEdit({ value: field.value, name: field.name })
  field.onBlur()
}}

// "should" be
onLoseFocus={name => {
  reportFieldEdit({ value: field.value, name })
  form.setFieldTouched(name)
}}

/>
{caption && <p className={captionCx}>{caption}</p>}
</div>
Expand Down
Loading