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(app): Enable pipette config modal and form #3202

Merged
merged 5 commits into from
Mar 13, 2019
Merged

feat(app): Enable pipette config modal and form #3202

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bd4fc69
feat(app): Enable pipette config modal and form
Kadee80 Mar 12, 2019
0c6c5a9
fixup: Cleanup unknown field logic and validation
Kadee80 Mar 13, 2019
362a9cd
fixup: Check max type
Kadee80 Mar 13, 2019
2f5716d
fixup: Consistent naming for unknown field consts and methods
Kadee80 Mar 13, 2019
bcbe0d7
fixup: Remove duplicate logic
Kadee80 Mar 13, 2019
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
37 changes: 32 additions & 5 deletions app/src/components/ConfigurePipette/ConfigForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ import {Formik, Form} from 'formik'

import startCase from 'lodash/startCase'
import mapValues from 'lodash/mapValues'
import forOwn from 'lodash/forOwn'
import keys from 'lodash/keys'
import pick from 'lodash/pick'
import omit from 'lodash/omit'
import set from 'lodash/set'
import forOwn from 'lodash/forOwn'
import isEmpty from 'lodash/isEmpty'

import FormButtonBar from './FormButtonBar'
Expand All @@ -33,6 +35,7 @@ type Props = {
pipette: Pipette,
pipetteConfig: PipetteConfigResponse,
updateConfig: (id: string, PipetteConfigRequest) => mixed,
showHiddenFields: boolean,
}

const PLUNGER_KEYS = ['top', 'bottom', 'blowout', 'dropTip']
Expand All @@ -57,13 +60,24 @@ export default class ConfigForm extends React.Component<Props> {
}

getVisibleFields = () => {
if (this.props.showHiddenFields) return this.props.pipetteConfig.fields
return pick(this.props.pipetteConfig.fields, [
...PLUNGER_KEYS,
...POWER_KEYS,
...TIP_KEYS,
])
}

getUnknownKeys = () => {
return keys(
omit(this.props.pipetteConfig.fields, [
...PLUNGER_KEYS,
...POWER_KEYS,
...TIP_KEYS,
])
)
}

handleSubmit = (values: FormValues) => {
const params = mapValues(values, v => {
return v === '' ? null : {value: Number(v)}
Expand All @@ -84,19 +98,24 @@ export default class ConfigForm extends React.Component<Props> {

validate = (values: FormValues) => {
const errors = {}
const fields = this.getVisibleFields()
const fields = this.props.showHiddenFields
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is duplicated here and in render and should probably be moved to getVisibleFields

Copy link
Contributor

Choose a reason for hiding this comment

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

This line change should be reverted with the update to getVisibleFields

? this.props.pipetteConfig.fields
: this.getVisibleFields()
const plungerFields = this.getFieldsByKey(PLUNGER_KEYS, fields)

// validate all visible fields with min and max
forOwn(fields, (field, name) => {
const value = values[name]
const value = values[name].trim()
const {min, max} = field

if (value !== '') {
const parsed = Number(value)
if (Number.isNaN(parsed)) {
set(errors, name, `number required`)
} else if (min && max && (parsed < min || value > max)) {
} else if (
typeof min === 'number' &&
max &&
Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably get the same typeof ... 'number' treatment, it's possible a max could be zero

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

(parsed < min || value > max)
) {
set(errors, name, `Min ${min} / Max ${max}`)
}
}
Expand All @@ -118,12 +137,14 @@ export default class ConfigForm extends React.Component<Props> {
render () {
const {parentUrl} = this.props
const fields = this.getVisibleFields()
const HIDDEN_KEYS = this.getUnknownKeys()
const initialValues = mapValues(fields, f => {
return f.value !== f.default ? f.value.toString() : ''
})
const plungerFields = this.getFieldsByKey(PLUNGER_KEYS, fields)
const powerFields = this.getFieldsByKey(POWER_KEYS, fields)
const tipFields = this.getFieldsByKey(TIP_KEYS, fields)
const devFields = this.getFieldsByKey(HIDDEN_KEYS, fields)

return (
<Formik
Expand Down Expand Up @@ -154,6 +175,12 @@ export default class ConfigForm extends React.Component<Props> {
groupLabel="Tip Pickup / Drop "
formFields={tipFields}
/>
{this.props.showHiddenFields && (
<ConfigFormGroup
groupLabel="For Dev Use Only"
formFields={devFields}
/>
)}
</FormColumn>
<FormButtonBar
buttons={[
Expand Down
6 changes: 5 additions & 1 deletion app/src/components/ConfigurePipette/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import * as React from 'react'
import {connect} from 'react-redux'
import {push} from 'react-router-redux'

import {
makeGetRobotPipettes,
makeGetRobotPipetteConfigs,
Expand All @@ -11,6 +10,7 @@ import {
makeGetPipetteRequestById,
} from '../../http-api-client'
import {chainActions} from '../../util'
import {getConfig} from '../../config'

import {getPipetteModelSpecs} from '@opentrons/shared-data'

Expand Down Expand Up @@ -39,6 +39,7 @@ type SP = {|
pipette: ?Pipette,
pipetteConfig: ?PipetteConfigResponse,
configError: ?ApiRequestError,
__featureEnabled: boolean,
|}

type DP = {|
Expand Down Expand Up @@ -72,6 +73,7 @@ function ConfigurePipette (props: Props) {
pipetteConfig={pipetteConfig}
parentUrl={parentUrl}
updateConfig={updateConfig}
showHiddenFields={props.__featureEnabled}
/>
)}
</ScrollableAlertModal>
Expand All @@ -93,10 +95,12 @@ function makeMapStateToProps (): (state: State, ownProps: OP) => SP {
pipette && configResponse && configResponse[pipette.id]
const configSetConfigCall =
pipette && getPipetteRequestById(state, ownProps.robot, pipette.id)
const devInternal = getConfig(state).devInternal
return {
pipette,
pipetteConfig,
configError: configSetConfigCall && configSetConfigCall.error,
__featureEnabled: !!devInternal && !!devInternal.allPipetteConfig,
}
}
}
Expand Down
8 changes: 0 additions & 8 deletions app/src/components/InstrumentSettings/AttachedPipettesCard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// attached pipettes container card
import * as React from 'react'
import {connect} from 'react-redux'
import {getIn} from '@thi.ng/paths'

import {
makeGetRobotPipettes,
Expand All @@ -20,7 +19,6 @@ import {Card, IntervalWrapper} from '@opentrons/components'
import type {State} from '../../types'
import type {Robot} from '../../discovery'
import type {Pipette} from '../../http-api-client'
import {getConfig} from '../../config'

type OP = Robot

Expand All @@ -29,7 +27,6 @@ type SP = {
left: ?Pipette,
right: ?Pipette,
showSettings: boolean,
__featureEnabled: boolean,
}

type DP = {
Expand All @@ -39,8 +36,6 @@ type DP = {

type Props = OP & SP & DP

const __FEATURE_FLAG = 'devInternal.newPipetteConfig'

const TITLE = 'Pipettes'

export default connect(
Expand All @@ -59,15 +54,13 @@ function AttachedPipettesCard (props: Props) {
{...props.left}
onChangeClick={props.clearMove}
showSettings={props.showSettings}
__enableConfig={props.__featureEnabled}
/>
<InstrumentInfo
mount="right"
name={props.name}
{...props.right}
onChangeClick={props.clearMove}
showSettings={props.showSettings}
__enableConfig={props.__featureEnabled}
/>
</CardContentFlex>
</Card>
Expand All @@ -88,7 +81,6 @@ function makeMapStateToProps (): (state: State, ownProps: OP) => SP {
left,
right,
showSettings: !!configCall.response,
__featureEnabled: !!getIn(getConfig(state), __FEATURE_FLAG),
}
}
}
Expand Down
3 changes: 1 addition & 2 deletions app/src/components/InstrumentSettings/InstrumentInfo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ type Props = {
name: string,
onChangeClick: () => mixed,
showSettings: boolean,
__enableConfig: boolean,
}

// TODO(mc, 2018-03-30): volume and channels should come from the API
Expand Down Expand Up @@ -54,7 +53,7 @@ export default function PipetteInfo (props: Props) {
<OutlineButton Component={Link} to={changeUrl} onClick={onChangeClick}>
{direction}
</OutlineButton>
{props.__enableConfig && model && showSettings && (
{model && showSettings && (
<OutlineButton Component={Link} to={configUrl}>
settings
</OutlineButton>
Expand Down
2 changes: 1 addition & 1 deletion app/src/config/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export type Config = {

// internal development flags
devInternal?: {
newPipetteConfig?: boolean,
allPipetteConfig?: boolean,
manualIp?: boolean,
},
}
Expand Down