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): implement liquids page interactivity #2478

Merged
merged 7 commits into from
Oct 16, 2018
Merged

feat(protocol-designer): implement liquids page interactivity #2478

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6a0ad8a
feat(protocol-designer): add liquid page edit form
IanLondon Oct 10, 2018
f831b26
feat(protocol-designer): implement liquids page interactivity
IanLondon Oct 12, 2018
678dcf4
i18n fixups
IanLondon Oct 15, 2018
5e022ef
fix copy case typo
IanLondon Oct 15, 2018
e95e3e5
fixup for PR comments: flow typing cleanup
IanLondon Oct 15, 2018
3b592f9
internationalization & flow type cleanups; disable save w/o changes
IanLondon Oct 15, 2018
2d8035c
cleanup :/
IanLondon Oct 16, 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
2 changes: 1 addition & 1 deletion protocol-designer/src/components/IngredientPropertiesForm.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
lost-column: 4/16;
}

.individualize {
.serialize {
Copy link
Contributor Author

@IanLondon IanLondon Oct 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

it was confusing to have old "individualize" word still hanging around, so I globally renamed it to "serialize" for consistency. Sorry for the extra diff

lost-column: 6/16;
}

Expand Down
14 changes: 7 additions & 7 deletions protocol-designer/src/components/IngredientPropertiesForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ class IngredientPropertiesForm extends React.Component<Props, State> {
name: null,
volume: null,
description: null,
individualize: false,
serialize: false,
},
commonIngredGroupId: null,
}
Expand All @@ -127,7 +127,7 @@ class IngredientPropertiesForm extends React.Component<Props, State> {
: this.state.commonIngredGroupId,
})
},
getSubstate: (inputKey) => this.state.input[inputKey],
getSubstate: (inputKey) => this.state.input[inputKey] || null,
})
}

Expand All @@ -143,15 +143,15 @@ class IngredientPropertiesForm extends React.Component<Props, State> {
const allIngredientGroupFields = (nextIngredGroupFields || this.props.allIngredientGroupFields || {})

if (ingredGroupId && ingredGroupId in allIngredientGroupFields) {
const {name, volume, description, individualize} = this.state.input
const {name, volume, description, serialize} = this.state.input
const newIngredFields = allIngredientGroupFields[ingredGroupId]
this.setState({
...this.state,
input: {
name: newIngredFields.name || name,
volume: newIngredFields.volume || volume,
description: newIngredFields.description || description,
individualize: newIngredFields.individualize || individualize,
serialize: newIngredFields.serialize || serialize,
},
}, cb)
} else {
Expand All @@ -166,7 +166,7 @@ class IngredientPropertiesForm extends React.Component<Props, State> {
name: null,
volume: null,
description: null,
individualize: false,
serialize: false,
},
}, cb)
}
Expand Down Expand Up @@ -290,11 +290,11 @@ class IngredientPropertiesForm extends React.Component<Props, State> {

<FormGroup
label={'\u00A0'} // non-breaking space
className={styles.individualize}
className={styles.serialize}
>
<Field
label='Serialize'
accessor='individualize'
accessor='serialize'
type='checkbox'
/>
</FormGroup>
Expand Down
4 changes: 2 additions & 2 deletions protocol-designer/src/components/IngredientsList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class IngredGroupCard extends React.Component<CardProps, CardState> {
groupId,
labwareWellContents,
} = this.props
const {individualize, description, name} = ingredGroup
const {serialize, description, name} = ingredGroup
const {isExpanded} = this.state

const wellsWithIngred = Object.keys(labwareWellContents)
Expand Down Expand Up @@ -91,7 +91,7 @@ class IngredGroupCard extends React.Component<CardProps, CardState> {
}

return <IngredIndividual key={well}
name={individualize
name={serialize
? `${ingredGroup.name || ''} ${i + 1}`
: ''
}
Expand Down
26 changes: 26 additions & 0 deletions protocol-designer/src/components/LiquidsPage/LiquidEditForm.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
@import '@opentrons/components';

.form_card {
margin: 1rem;
padding: 1rem;
}

.section {
padding-bottom: 2rem;
}

.info_text {
padding-bottom: 1.5rem;
}

.button_row {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

one day, we will not need to keep writing .button_rows

lost-utility: clearfix;

& > * {
lost-column: 1/6;
}

& > *:nth-child(2) {
lost-offset: 3/6;
}
}
143 changes: 143 additions & 0 deletions protocol-designer/src/components/LiquidsPage/LiquidEditForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
// @flow
import * as React from 'react'
import {connect} from 'react-redux'
import assert from 'assert'
import i18n from '../../localization'

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

import {
Card,
CheckboxField,
FormGroup,
InputField,
OutlineButton,
PrimaryButton,
} from '@opentrons/components'
import styles from './LiquidEditForm.css'
import formStyles from '../forms.css'

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

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

type SP = {
...IngredInputs,
_liquidGroupId: ?string,
showForm: 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,
}
}

updateForm = (fieldName: $Keys<IngredInputs>) => (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)
}

render () {
const {deleteLiquidGroup, cancelForm} = this.props
const {name, description, serialize} = this.state
return (
<Card className={styles.form_card}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a change request, but a similar comment to the one I left the other day: this component (referring here to the entire file) is mixing a lot of presentational stuff, stateful stuff, and redux connection stuff. I think there's some opportunities here for:

  • Making the form state a more generic non-presentational component
  • Making the form layout a more generic presentational component
  • Composing the specific redux-connected LiquidEditForm from those two components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you have an example of this anywhere eg in Run App? I'm not sure how to do with without a bunch of boilerplate (or maybe, if I see it I'll see the boilerplate isn't as bad as I think)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that form state (which is transient) is closer to UI/presentational logic than it is to domain layer/ non-presentational logic here. The React state in this component is tracking a UI concern, the presentational state of these interactive components. Their domain layer interface is really just saving/deleting the form or navigating away. I think the concerns are clearly organized in this file. If separating the domain layer connection to a new sibling file is clearer for @mcous, I would be fine with that, though I think it is actually clearer with the concerns collocated.

As far as building out a component for removing boilerplate from our stateful form components, I've built a couple before and I think we could definitely benefit from throwing one in our component library, or using a third party open source component. Slotting in time to build one out seems like the limiting factor here, especially as we would probably want to slowly migrate all of our forms over.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm maybe the cards in app/src/components/RobotSettings, starting in the presentational component in index.js? Otherwise there's the the ConfigureWifiForm in app/src/components/RobotSettings/WifiConnectForm.js (just now noticing we messed up the filename on that...) that does a little bit of generic form logic vs specific form logic split. Not sure where else to point to, because...

...In the app we generally don't import redux and css into the same file (this might be true across the board but I haven't checked). This is a heavy-handed approach, so I'm not necessarily recommending it, but it's the approach I like to use. It's not a panacea, though; we've definitely got components in the app that mix-concerns in spite of this.

I get the feeling that these sorts of container/presentation splits would be more immediately obvious if we were better about unit testing the React side of things, so (since last week) I've been trying to at least think about how I would test my components if I was writing tests when I encounter these sorts of "where to split" questions

<section className={styles.section}>
<div className={formStyles.header}>{i18n.t('liquids.details')}</div>
<div className={formStyles.row_wrapper}>
<FormGroup
label={`${i18n.t('liquids.liquid_name')}:`}
className={formStyles.column_1_2}>
<InputField value={name} onChange={this.updateForm('name')} />
</FormGroup>
<FormGroup
label={`${i18n.t('liquids.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('liquids.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 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: IngredInputs) => dispatch(labwareIngredActions.editLiquidGroup({
...formData,
liquidGroupId: _liquidGroupId,
})),
},
}
}

export default connect(mapStateToProps, null, mergeProps)(LiquidEditFormWrapper)
3 changes: 2 additions & 1 deletion protocol-designer/src/components/LiquidsPage/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// @flow
import * as React from 'react'
import LiquidEditForm from './LiquidEditForm'

export default function LiquidsPage () {
return <div>TODO! This page will be implemented in the next ticket (#2427)</div>
return <LiquidEditForm />
}
29 changes: 23 additions & 6 deletions protocol-designer/src/components/LiquidsSidebar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,24 @@
// @flow
import * as React from 'react'
import {connect} from 'react-redux'
import {SidePanel, swatchColors} from '@opentrons/components'
import {
PrimaryButton,
SidePanel,
swatchColors,
} from '@opentrons/components'
import {PDTitledList} from '../lists'
import listButtonStyles from '../listButtons.css'

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

type Props = {
liquids: OrderedLiquids,
selectedLiquid: ?string,
handleClickLiquid: (liquidId: string) => () => mixed,
createNewLiquid: () => mixed,
selectLiquid: (liquidId: string) => mixed,
}

type SP = {
Expand All @@ -22,33 +29,43 @@ type SP = {
type DP = $Diff<Props, SP>

function LiquidsSidebar (props: Props) {
const {liquids, selectedLiquid, handleClickLiquid} = props
const {liquids, selectedLiquid, createNewLiquid, selectLiquid} = props
return (
<SidePanel title='Liquids'>
{liquids.map(({ingredientId, name}) => (
<PDTitledList
key={ingredientId}
selected={selectedLiquid === ingredientId}
onClick={handleClickLiquid(ingredientId)}
onClick={() => selectLiquid(ingredientId)}
Copy link
Contributor

Choose a reason for hiding this comment

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

We could reduce redefining this function for every liquid in the sidebar on each render by making this a class component and binding makeHandleClickIngredient(ingredientId) that would return () => selectLiquid(ingredientId)

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 not sure how to benchmark this with any accuracy b/c its so cheap in performance that running the benchmark loop itself is a majority of the compute cost, but with this:

d = Date.now();
for(var x=0; x < 2000000; x++) {
  const aaa = () => {};
}
console.log(Date.now() - d);

2 million assignments takes ~1 second on my computer, and without the const aaa = () => {} the 2M empty for loop takes ~800ms, so the 2M function assignment and garbage collection takes roughly ~200ms?

So personally I think the whole function-binding-optimization thing is a premature optimization and not worth the code ugliness -- especially if React ever delivers on its promise of optimizing pure functional components 🤞

iconName='circle'
iconProps={{style: {fill: swatchColors(Number(ingredientId))}}}
title={name || `Unnamed Ingredient ${ingredientId}`} // fallback, should not happen
/>
))}
<div className={listButtonStyles.list_item_button}>
<PrimaryButton
iconName='water'
onClick={createNewLiquid}>
New Liquid
Copy link
Contributor

Choose a reason for hiding this comment

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

should be in i18n

</PrimaryButton>
</div>
</SidePanel>
)
}

function mapStateToProps (state: BaseState): SP {
const selectedLiquidGroup = labwareIngredSelectors.getSelectedLiquidGroupState(state)
return {
liquids: labwareIngredSelectors.allIngredientNamesIds(state),
selectedLiquid: '0', // TODO: Ian 2018-10-09 implement in #2427
selectedLiquid: selectedLiquidGroup && selectedLiquidGroup.liquidGroupId,
}
}

function mapDispatchToProps (dispatch: Dispatch<*>): DP {
return {
handleClickLiquid: (liquidId) => () => console.log('TODO: select liquid', liquidId), // TODO: Ian 2018-10-09 implement in #2427
selectLiquid: (liquidGroupId) =>
dispatch(labwareIngredActions.selectLiquidGroup(liquidGroupId)),
createNewLiquid: () => dispatch(labwareIngredActions.createNewLiquidGroup()),
}
}

Expand Down
4 changes: 2 additions & 2 deletions protocol-designer/src/components/StepCreationButton.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 styles from './StepCreationButton.css'
import styles from './listButtons.css'
import i18n from '../localization'

import {HoverTooltip, PrimaryButton} from '@opentrons/components'
Expand Down Expand Up @@ -38,7 +38,7 @@ function StepCreationButton (props: Props) {
)

return (
<div className={styles.step_creation_button} onMouseLeave={onClickAway}>
<div className={styles.list_item_button} onMouseLeave={onClickAway}>
<PrimaryButton
onClick={expanded ? onClickAway : onExpandClick}
>
Expand Down
3 changes: 2 additions & 1 deletion protocol-designer/src/components/StepEditForm/ButtonRow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {OutlineButton, PrimaryButton} from '@opentrons/components'
import {actions, selectors} from '../../steplist'
import type {BaseState, ThunkDispatch} from '../../types'
import styles from './StepEditForm.css'
import formStyles from '../Form.css'

type OP = {onDelete?: (event: SyntheticEvent<>) => mixed}
type SP = {canSave?: ?boolean}
Expand All @@ -19,7 +20,7 @@ type Props = OP & SP & DP
const ButtonRow = (props: Props) => {
const {canSave, onDelete, onSave, onCancel, onClickMoreOptions} = props
return (
<div className={styles.button_row}>
<div className={formStyles.button_row}>
<OutlineButton onClick={onDelete}>DELETE</OutlineButton>
<OutlineButton onClick={onClickMoreOptions}>NOTES</OutlineButton>
<PrimaryButton className={styles.cancel_button} onClick={onCancel}>CANCEL</PrimaryButton>
Expand Down
9 changes: 0 additions & 9 deletions protocol-designer/src/components/StepEditForm/StepEditForm.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,6 @@

/* form buttons */

.button_row {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this most generic .button_row (out of ~7) into the more generic Forms.css

lost-utility: clearfix;
margin-top: 2rem;

& > * {
lost-column: 1/6;
}
}

.cancel_button {
lost-offset: 2/6;
}
Expand Down
Loading