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(protocol-designer): use formik for liquid edit form #2512

Merged
merged 2 commits into from
Oct 18, 2018
Merged

feat(protocol-designer): use formik for liquid edit form #2512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1172065
feat(protocol-designer): use formik for liquid edit form
IanLondon Oct 17, 2018
10087e8
fixup: add max-width to instructional text
IanLondon Oct 18, 2018
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
3 changes: 3 additions & 0 deletions components/src/__tests__/__snapshots__/forms.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ exports[`CheckboxField renders correctly when checked 1`] = `
checked={true}
className="input_field accessibly_hidden"
disabled={undefined}
name={undefined}
onChange={undefined}
type="checkbox"
/>
Expand Down Expand Up @@ -69,6 +70,7 @@ exports[`CheckboxField renders correctly when unchecked 1`] = `
checked={false}
className="input_field accessibly_hidden"
disabled={undefined}
name={undefined}
onChange={undefined}
type="checkbox"
/>
Expand Down Expand Up @@ -253,6 +255,7 @@ exports[`InputField renders correctly 1`] = `
>
<input
autoFocus={undefined}
name={undefined}
onBlur={undefined}
onChange={undefined}
onClick={undefined}
Expand Down
3 changes: 3 additions & 0 deletions components/src/forms/CheckboxField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ type Props = {
value?: boolean,
/** classes to apply */
className?: string,
/** name of field in form */
name?: string,
/** label text for checkbox */
label?: string,
/** if is included, checkbox will use error style. The content of the string is ignored. */
Expand Down Expand Up @@ -45,6 +47,7 @@ export default function CheckboxField (props: Props) {
<input
className={cx(styles.input_field, styles.accessibly_hidden)}
type='checkbox'
name={props.name}
checked={props.value || false}
disabled={props.disabled}
onChange={props.onChange}
Expand Down
3 changes: 3 additions & 0 deletions components/src/forms/InputField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ type Props = {
className?: string,
/** inline label text. DEPRECATED */
label?: string,
/** name of field in form */
name?: string,
/** placeholder text */
placeholder?: string,
/** optional units string, appears to the right of input text */
Expand Down Expand Up @@ -83,6 +85,7 @@ function Input (props: Props) {
<input
type={props.type || 'text'}
value={props.value || ''}
name={props.name}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey so @Kadee80 and I happen to have made a similar change on a WIP branch. We need to put some inputs outside of their labels in the DOM, so we're also proposing that name is assigned to input.id. Does that sound ok?

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 think Formik will handle that fine, let me try it out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From Slack convo: in an upcoming Run App PR, we'll add id as a pass-thru prop like <input id={props.id} to our Input components, and not require it, and not force it to be the same as name

placeholder={props.placeholder}
onChange={props.disabled ? undefined : props.onChange}
onFocus={props.disabled ? undefined : props.onFocus}
Expand Down
67 changes: 67 additions & 0 deletions flow-typed/npm/formik_vx.x.x.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// flow-typed signature: 2eb43e07f430c2c2afdede8041c6d9d3
// flow-typed version: <<STUB>>/formik_v1.3.1/flow_v0.82.0

/**
* This is an autogenerated libdef stub for:
*
* 'formik'
*
* Fill this stub out by replacing all the `any` types.
*
* Once filled out, we encourage you to share your work with the
* community by sending a pull request to:
* https://github.com/flowtype/flow-typed
*/

declare module 'formik' {
declare module.exports: any;
}

/**
* We include stubs for each file inside this npm package in case you need to
* require those files directly. Feel free to delete any files that aren't
* needed.
*/
declare module 'formik/dist/formik.cjs.development' {
declare module.exports: any;
}

declare module 'formik/dist/formik.cjs.production' {
declare module.exports: any;
}

declare module 'formik/dist/formik.esm' {
declare module.exports: any;
}

declare module 'formik/dist/formik.umd.development' {
declare module.exports: any;
}

declare module 'formik/dist/formik.umd.production' {
declare module.exports: any;
}

declare module 'formik/dist/index' {
declare module.exports: any;
}

// Filename aliases
declare module 'formik/dist/formik.cjs.development.js' {
declare module.exports: $Exports<'formik/dist/formik.cjs.development'>;
}
declare module 'formik/dist/formik.cjs.production.js' {
declare module.exports: $Exports<'formik/dist/formik.cjs.production'>;
}
declare module 'formik/dist/formik.esm.js' {
declare module.exports: $Exports<'formik/dist/formik.esm'>;
}
declare module 'formik/dist/formik.umd.development.js' {
declare module.exports: $Exports<'formik/dist/formik.umd.development'>;
}
declare module 'formik/dist/formik.umd.production.js' {
declare module.exports: $Exports<'formik/dist/formik.umd.production'>;
}
declare module 'formik/dist/index.js' {
declare module.exports: $Exports<'formik/dist/index'>;
}
4 changes: 3 additions & 1 deletion protocol-designer/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"dependencies": {
"@opentrons/components": "3.5.0-beta.1",
"classnames": "^2.2.5",
"formik": "^1.3.1",
"i18next": "^11.5.0",
"lodash": "^4.17.4",
"prop-types": "^15.6.0",
Expand All @@ -32,7 +33,8 @@
"redux-actions": "^2.2.1",
"redux-thunk": "^2.2.0",
"reselect": "^3.0.1",
"uuid": "^3.3.2"
"uuid": "^3.3.2",
"yup": "^0.26.6"
},
"devDependencies": {
"flow-bin": "^0.82.0",
Expand Down
211 changes: 82 additions & 129 deletions protocol-designer/src/components/LiquidsPage/LiquidEditForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,8 @@
// @flow
import * as React from 'react'
import {connect} from 'react-redux'
import assert from 'assert'
import {Formik} from 'formik'
import * as Yup from 'yup'
import i18n from '../../localization'

import * as labwareIngredActions from '../../labware-ingred/actions'
import {selectors as labwareIngredSelectors} from '../../labware-ingred/reducers'
import type {LiquidGroup} from '../../labware-ingred/types'
import type {BaseState, ThunkDispatch} from '../../types'

import {
Card,
CheckboxField,
Expand All @@ -19,136 +13,95 @@ import {
} from '@opentrons/components'
import styles from './LiquidEditForm.css'
import formStyles from '../forms.css'
import type {LiquidGroup} from '../../labware-ingred/types'

type Props = {
...$Exact<LiquidGroup>,
deleteLiquidGroup: () => mixed,
cancelForm: () => mixed,
saveForm: (LiquidGroup) => mixed,
}
type State = LiquidGroup

type WrapperProps = {showForm: boolean, formKey: string, formProps: Props}

type SP = {
...LiquidGroup,
_liquidGroupId: ?string,
showForm: boolean,
type LiquidEditFormValues = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we use this style, this type is important to pay attention to when maintaining form code. It must exactly match the type of the Yup schema, otherwise when the form communicates its values (eg dispatches some submitForm action) on the reciever's end the values could be different than what Flow expects, but Flow won't be able to catch it since the form values are untyped.

name: string,
description?: ?string,
serialize?: boolean,
}

class LiquidEditForm extends React.Component<Props, State> {
constructor (props: Props) {
super(props)
this.state = {
name: props.name,
description: props.description,
serialize: props.serialize || false,
}
export const liquidEditFormSchema = Yup.object().shape({
name: Yup.string().required('Name is required'),
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be nice to throw these validation strings into i18n

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, will do in follow-up PR, I wanna make sure I'm putting them in a place that works for multiple forms when I do it

description: Yup.string(),
serialize: Yup.boolean(),
})

export default function LiquidEditForm (props: Props) {
const {deleteLiquidGroup, cancelForm, saveForm} = props

const initialValues = {
name: props.name,
description: props.description || '',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this "cast null to empty string", the form will be seen as "dirty" if you click into description field and click out of it. That's because the initial value is technically different from the current value (null !== '')

serialize: props.serialize || false,
}

updateForm = (fieldName: $Keys<LiquidGroup>) => (e: SyntheticInputEvent<*>) => {
// need to handle checkbox fields explicitly
if (fieldName === 'serialize') {
this.setState({[fieldName]: !this.state[fieldName]})
} else {
this.setState({[fieldName]: e.currentTarget.value})
}
}

handleSaveForm = (e: SyntheticMouseEvent<*>) => {
this.props.saveForm(this.state)
}

disableSave = () => {
const hasChanges = Object.keys(this.state).some(field =>
(this.state[field] || null) !== (this.props[field] || null)
)
return !hasChanges
}

render () {
const {deleteLiquidGroup, cancelForm} = this.props
const {name, description, serialize} = this.state
return (
<Card className={styles.form_card}>
<section className={styles.section}>
<div className={formStyles.header}>{i18n.t('form.liquid.details')}</div>
<div className={formStyles.row_wrapper}>
<FormGroup
label={`${i18n.t('form.liquid.name')}:`}
className={formStyles.column_1_2}>
<InputField value={name} onChange={this.updateForm('name')} />
</FormGroup>
<FormGroup
label={`${i18n.t('form.liquid.description')}:`}
className={formStyles.column_1_2}>
<InputField value={description} onChange={this.updateForm('description')} />
</FormGroup>
</div>
</section>

<section className={styles.section}>
<div className={formStyles.header}>Serialization</div>
<p className={styles.info_text}>
{i18n.t('form.liquid.serialize_explanation')}</p>
<CheckboxField label='Serialize' value={serialize}
onChange={this.updateForm('serialize')} />
</section>

<div className={styles.button_row}>
<OutlineButton onClick={deleteLiquidGroup}>{i18n.t('button.delete')}</OutlineButton>
<PrimaryButton onClick={cancelForm}>{i18n.t('button.cancel')}</PrimaryButton>
<PrimaryButton
disabled={this.disableSave()}
onClick={this.handleSaveForm}>
{i18n.t('button.save')}
</PrimaryButton>
</div>
</Card>
)
}
}

function LiquidEditFormWrapper (props: WrapperProps) {
const {showForm, formKey, formProps} = props
return showForm
? <LiquidEditForm {...formProps} key={formKey} />
: null
}

function mapStateToProps (state: BaseState): SP {
const selectedLiquidGroupState = labwareIngredSelectors.getSelectedLiquidGroupState(state)
const _liquidGroupId = (selectedLiquidGroupState && selectedLiquidGroupState.liquidGroupId)
const allIngredientGroupFields = labwareIngredSelectors.allIngredientGroupFields(state)
const selectedIngredFields = _liquidGroupId ? allIngredientGroupFields[_liquidGroupId] : {}
const showForm = Boolean(selectedLiquidGroupState.liquidGroupId || selectedLiquidGroupState.newLiquidGroup)
assert(!(_liquidGroupId && !selectedIngredFields), `Expected selected liquid group "${String(_liquidGroupId)}" to have fields in allIngredientGroupFields`)

return {
_liquidGroupId,
showForm,
name: selectedIngredFields.name,
description: selectedIngredFields.description,
serialize: selectedIngredFields.serialize,
}
}

function mergeProps (stateProps: SP, dispatchProps: {dispatch: ThunkDispatch<*>}): WrapperProps {
const {dispatch} = dispatchProps
const {showForm, _liquidGroupId, ...passThruFormProps} = stateProps
return {
showForm,
formKey: _liquidGroupId || '__new_form__',
formProps: {
...passThruFormProps,
deleteLiquidGroup: () => window.alert('Deleting liquids is not yet implemented'), // TODO: Ian 2018-10-12 later ticket
cancelForm: () => dispatch(labwareIngredActions.deselectLiquidGroup()),
saveForm: (formData: LiquidGroup) => dispatch(labwareIngredActions.editLiquidGroup({
...formData,
liquidGroupId: _liquidGroupId,
})),
},
}
return (
<Formik
initialValues={initialValues}
validationSchema={liquidEditFormSchema}
onSubmit={(values: LiquidEditFormValues) => saveForm({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note that I am typing the untyped values using LiquidEditFormValues, which matches the Yup validation schema. Without this, it's easy to pass invalid values from the form to the outside world.

name: values.name,
description: values.description || null,
serialize: values.serialize || false,
})}
render={({handleChange, handleBlur, handleSubmit, dirty, errors, isValid, touched, values}) => (
<Card className={styles.form_card}>
<form onSubmit={handleSubmit}>
<section className={styles.section}>
<div className={formStyles.header}>{i18n.t('form.liquid.details')}</div>
<div className={formStyles.row_wrapper}>
<FormGroup
label={`${i18n.t('form.liquid.name')}:`}
className={formStyles.column_1_2}>
<InputField
name='name'
error={touched.name && errors.name}
value={values.name}
onChange={handleChange}
onBlur={handleBlur}
/>
</FormGroup>
<FormGroup
label={`${i18n.t('form.liquid.description')}:`}
className={formStyles.column_1_2}>
<InputField
name='description'
value={values.description}
onChange={handleChange} />
</FormGroup>
</div>
</section>

<section className={styles.section}>
<div className={formStyles.header}>{i18n.t('form.liquid.serialize_title')}</div>
<p className={styles.info_text}>
{i18n.t('form.liquid.serialize_explanation')}</p>
<CheckboxField
name='serialize'
label={i18n.t('form.liquid.serialize')}
value={values.serialize}
onChange={handleChange} />
</section>

<div className={styles.button_row}>
<OutlineButton onClick={deleteLiquidGroup}>{i18n.t('button.delete')}</OutlineButton>
<PrimaryButton onClick={cancelForm}>{i18n.t('button.cancel')}</PrimaryButton>
<PrimaryButton
disabled={!dirty}
Copy link
Contributor Author

@IanLondon IanLondon Oct 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In Formik, dirty means !deepEqual(initialValues, values). You can fill out an entire form and then change all the fields back to how they were initially, and if it matches then the form is no longer dirty. The dirty value doesn't have anything to do with "touched" (which is set with onBlur={handleBlur})

type='submit' >
{i18n.t('button.save')}
</PrimaryButton>
</div>
</form>
</Card>
)}
/>
)
}

export default connect(mapStateToProps, null, mergeProps)(LiquidEditFormWrapper)
Loading