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

Pd file detail form edit #1673

Merged
merged 9 commits into from
Jun 15, 2018
Merged

Pd file detail form edit #1673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c57872f
get a button and some more selectors out there
b-cooper Jun 7, 2018
db9d6a2
wire up the save
b-cooper Jun 8, 2018
de4b90a
move towards correct file name
b-cooper Jun 8, 2018
91b684d
enter key press handler
b-cooper Jun 8, 2018
2339dde
feat(protocol-designer): clarify editing file details
b-cooper Jun 8, 2018
a3c4e57
onKeyDown flow type
b-cooper Jun 11, 2018
4955d20
[fix-up] address ian pr comments
b-cooper Jun 11, 2018
7c83585
[fixup] morgan nit and ian comment addressed
b-cooper Jun 11, 2018
28af4f0
remove stale TODO
b-cooper Jun 15, 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
10 changes: 10 additions & 0 deletions protocol-designer/src/components/FilePage.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,13 @@
.file_page section {
margin: 0 2.5rem;
}

.button_row {
display: flex;
justify-content: flex-end;
}

.update_button {
width: 8rem;
align-self: flex-start;
}
56 changes: 37 additions & 19 deletions protocol-designer/src/components/FilePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,55 @@
// @flow
import * as React from 'react'
import {FormGroup, InputField, InstrumentGroup} from '@opentrons/components'
import type {FilePageFields} from '../file-data'
import {
FormGroup,
InputField,
InstrumentGroup,
OutlineButton
} from '@opentrons/components'
import type {FileMetadataFields} from '../file-data'
import type {FormConnector} from '../utils'

import styles from './FilePage.css'
import formStyles from '../components/forms.css'

type Props = {
formConnector: FormConnector<FilePageFields>,
instruments: React.ElementProps<typeof InstrumentGroup>
export type FilePageProps = {
formConnector: FormConnector<FileMetadataFields>,
isFormAltered: boolean,
instruments: React.ElementProps<typeof InstrumentGroup>,
saveFileMetadata: () => void
}

export default function FilePage (props: Props) {
const {formConnector, instruments} = props
const FilePage = ({formConnector, isFormAltered, instruments, saveFileMetadata}: FilePageProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why the switch to an anonymous arrow function? Also, personally not a huge fan of object destructuring inside function parameters (especially when flow gets involved).

Both of these are highly personal readability preferences, so this comment shouldn't be construed as a change request. But figured I'd bring it up because we were chatting about this stuff earlier

Copy link
Contributor

@IanLondon IanLondon Jun 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I agree about avoiding destructuring inside fn params, especially of props. We're pretty consistent about that convention across all our components

const handleSubmit = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be a good part of a generic Form component in complib. Also, should this be a method of a class based component instead to avoid the new function on every render? (Probably a premature optimization)

// blur focused field on submit
if (document && document.activeElement) document.activeElement.blur()
saveFileMetadata()
}
return (
<div className={styles.file_page}>
<section>
<h2>
Information
</h2>
<form onSubmit={handleSubmit}>
<div className={formStyles.row_wrapper}>
<FormGroup label='Protocol Name:' className={formStyles.column_1_2}>
<InputField placeholder='Untitled' {...formConnector('name')} />
</FormGroup>

<div className={formStyles.row_wrapper}>
<FormGroup label='Protocol Name:' className={formStyles.column_1_2}>
<InputField placeholder='Untitled' {...formConnector('name')} />
</FormGroup>
<FormGroup label='Organization/Author:' className={formStyles.column_1_2}>
<InputField {...formConnector('author')} />
</FormGroup>
</div>

<FormGroup label='Organization/Author:' className={formStyles.column_1_2}>
<InputField {...formConnector('author')} />
<FormGroup label='Description:'>
<InputField {...formConnector('description')}/>
</FormGroup>
</div>

<FormGroup label='Description:'>
<InputField {...formConnector('description')}/>
</FormGroup>
<div className={styles.button_row}>
<OutlineButton type="submit" className={styles.update_button} disabled={!isFormAltered}>
{isFormAltered ? 'UPDATE' : 'UPDATED'}
</OutlineButton>
</div>
</form>
</section>

<section>
Expand All @@ -45,3 +61,5 @@ export default function FilePage (props: Props) {
</div>
)
}

export default FilePage
51 changes: 28 additions & 23 deletions protocol-designer/src/containers/ConnectedFilePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,55 +1,60 @@
// @flow
import * as React from 'react'
import type {Dispatch} from 'redux'
import {connect} from 'react-redux'
import type {BaseState} from '../types'

import FilePage from '../components/FilePage'

import type {FilePageProps} from '../components/FilePage'
import {actions, selectors} from '../file-data'
import type {FilePageFields} from '../file-data'
import type {FileMetadataFields} from '../file-data'
import {formConnectorFactory, type FormConnector} from '../utils'

export default connect(mapStateToProps, null, mergeProps)(FilePage)

type Props = React.ElementProps<typeof FilePage>
type StateProps = {
instruments: $PropertyType<Props, 'instruments'>,
type SP = {
instruments: $PropertyType<FilePageProps, 'instruments'>,
isFormAltered: boolean,
_values: {[string]: string}
}

function mapStateToProps (state: BaseState): StateProps {
const formValues = selectors.fileFormValues(state)
const pipetteData = selectors.pipettesForInstrumentGroup(state)
type DP = {
_updateFileMetadataFields: typeof actions.updateFileMetadataFields,
_saveFileMetadata: ({[string]: string}) => void
}

const mapStateToProps = (state: BaseState): SP => {
const pipetteData = selectors.pipettesForInstrumentGroup(state)
return {
_values: formValues,
_values: selectors.fileFormValues(state),
isFormAltered: selectors.isUnsavedMetadatFormAltered(state),
instruments: {
left: pipetteData.find(i => i.mount === 'left'),
right: pipetteData.find(i => i.mount === 'right')
}
}
}

function mergeProps (
stateProps: StateProps,
dispatchProps: {dispatch: Dispatch<*>}
): Props {
const {instruments, _values} = stateProps
const {dispatch} = dispatchProps
const mapDispatchToProps = {
_updateFileMetadataFields: actions.updateFileMetadataFields,
_saveFileMetadata: actions.saveFileMetadata
}

const mergeProps = (
{instruments, isFormAltered, _values}: SP,
{_updateFileMetadataFields, _saveFileMetadata}: DP
): FilePageProps => {
const onChange = (accessor) => (e: SyntheticInputEvent<*>) => {
if (accessor === 'name' || accessor === 'description' || accessor === 'author') {
dispatch(actions.updateFileFields({[accessor]: e.target.value}))
_updateFileMetadataFields({[accessor]: e.target.value})
} else {
console.warn('Invalid accessor in ConnectedFilePage:', accessor)
}
}

const formConnector: FormConnector<FilePageFields> = formConnectorFactory(onChange, _values)
const formConnector: FormConnector<FileMetadataFields> = formConnectorFactory(onChange, _values)

return {
formConnector,
instruments
isFormAltered,
instruments,
saveFileMetadata: () => _saveFileMetadata(_values)
}
}

export default connect(mapStateToProps, mapDispatchToProps, mergeProps)(FilePage)
2 changes: 1 addition & 1 deletion protocol-designer/src/containers/ConnectedFileSidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ type MP = {
export default connect(mapStateToProps, null, mergeProps)(FileSidebar)

function mapStateToProps (state: BaseState): SP & MP {
const protocolName = fileDataSelectors.fileFormValues(state).name || 'untitled'
const protocolName = fileDataSelectors.fileMetadata(state).name || 'untitled'
const fileData = fileDataSelectors.createFile(state)

return {
Expand Down
5 changes: 3 additions & 2 deletions protocol-designer/src/containers/ConnectedNewFileModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@ function mapDispatchToProps (dispatch: Dispatch<*>): DispatchProps {
return {
onCancel: () => dispatch(navigationActions.toggleNewProtocolModal(false)),
onSave: fields => {
dispatch(fileActions.updateFileFields({
name: fields.name || ''
dispatch(fileActions.saveFileMetadata({
name: fields.name || '',
description: ''
}))

dispatch(fileActions.updatePipettes({
Expand Down
6 changes: 2 additions & 4 deletions protocol-designer/src/containers/ConnectedTitleBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,19 @@ import {TitleBar, humanizeLabwareType} from '@opentrons/components'

import {selectors as labwareIngredSelectors} from '../labware-ingred/reducers'
import {selectors as steplistSelectors} from '../steplist/reducers'
import {selectors as fileDataSelectors} from '../file-data'
import {closeIngredientSelector} from '../labware-ingred/actions'

import {selectors, type Page} from '../navigation'
import type {BaseState} from '../types'

type Props = React.ElementProps<typeof TitleBar>

type DP = { onBackClick: $PropertyType<Props, 'onBackClick'> }

type SP = $Diff<Props, DP> & {_page: Page}

function mapStateToProps (state: BaseState): SP {
const _page = selectors.currentPage(state)
// TODO: Ian 2018-02-22 fileName from file
const fileName = 'Protocol Name'
const fileName = fileDataSelectors.protocolName(state)

switch (_page) {
case 'file-splash':
Expand Down
13 changes: 8 additions & 5 deletions protocol-designer/src/file-data/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
// @flow
import type {FilePageFieldAccessors} from './types'
import type {FileMetadataFieldAccessors} from './types'
import type {PipetteName} from './pipetteData'

// NOTE: updateFileFields contains pipette name identifiers, though pipettes aren't file fields.
// This is because both pipettes and file fields can get changed in the same place
export const updateFileFields = (payload: {[accessor: FilePageFieldAccessors]: string}) => ({
type: 'UPDATE_FILE_FIELDS',
export const updateFileMetadataFields = (payload: {[accessor: FileMetadataFieldAccessors]: string}) => ({
type: 'UPDATE_FILE_METADATA_FIELDS',
payload
})

export const saveFileMetadata = (payload: {[accessor: FileMetadataFieldAccessors]: string}) => ({
type: 'SAVE_FILE_METADATA',
payload
})

Expand Down
25 changes: 19 additions & 6 deletions protocol-designer/src/file-data/reducers/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,32 @@
import {combineReducers} from 'redux'
import {handleActions, type ActionType} from 'redux-actions'

import {updateFileFields, updatePipettes} from '../actions'
import {updateFileMetadataFields, updatePipettes, saveFileMetadata} from '../actions'
import {pipetteDataByName, type PipetteName} from '../pipetteData'

import type {Mount} from '@opentrons/components'
import type {PipetteData} from '../../step-generation'
import type {FilePageFields} from '../types'
import type {FileMetadataFields} from '../types'

const defaultFields = {
name: '',
author: '',
description: ''
}

const metadataFields = handleActions({
UPDATE_FILE_FIELDS: (state: FilePageFields, action: ActionType<typeof updateFileFields>) => ({
const unsavedMetadataForm = handleActions({
UPDATE_FILE_METADATA_FIELDS: (state: FileMetadataFields, action: ActionType<typeof updateFileMetadataFields>) => ({
...state,
...action.payload
}),
SAVE_FILE_METADATA: (state: FileMetadataFields, action: ActionType<typeof saveFileMetadata>) => ({
...state,
...action.payload
})
}, defaultFields)

const fileMetadata = handleActions({
SAVE_FILE_METADATA: (state: FileMetadataFields, action: ActionType<typeof saveFileMetadata>) => ({
...state,
...action.payload
})
Expand Down Expand Up @@ -58,12 +69,14 @@ const pipettes = handleActions({
}, {left: null, right: null})

export type RootState = {
metadataFields: FilePageFields,
unsavedMetadataForm: FileMetadataFields,
fileMetadata: FileMetadataFields,
pipettes: PipetteState
}

const _allReducers = {
metadataFields,
unsavedMetadataForm,
fileMetadata,
pipettes
}

Expand Down
10 changes: 5 additions & 5 deletions protocol-designer/src/file-data/selectors/fileCreator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,20 @@ import mapValues from 'lodash/mapValues'
import type {BaseState} from '../../types'
import type {ProtocolFile, FilePipette, FileLabware} from '../types'
import type {LabwareData, PipetteData} from '../../step-generation'
import {fileFormValues} from './fileFields'
import {fileMetadata} from './fileFields'
import {getInitialRobotState, robotStateTimeline} from './commands'

// TODO LATER Ian 2018-02-28 deal with versioning
const protocolSchemaVersion = '1.0.0'
const applicationVersion = '1.0.0'

export const createFile: BaseState => ?ProtocolFile = createSelector(
fileFormValues,
fileMetadata,
getInitialRobotState,
robotStateTimeline,
(fileFormValues, initialRobotState, _robotStateTimeline) => {
const {author, description} = fileFormValues
const name = fileFormValues.name || 'untitled'
(fileMetadata, initialRobotState, _robotStateTimeline) => {
const {author, description} = fileMetadata
const name = fileMetadata.name || 'untitled'
const isValidFile = true // TODO Ian 2018-02-28 this will be its own selector

if (!isValidFile) {
Expand Down
14 changes: 13 additions & 1 deletion protocol-designer/src/file-data/selectors/fileFields.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// @flow
import _ from 'lodash'
Copy link
Contributor

Choose a reason for hiding this comment

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

We try to import specifically what we're using from lodash:

import isEqual from 'lodash/isEqual'

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch!

import {createSelector} from 'reselect'
import type {BaseState} from '../../types'
import type {RootState} from '../reducers'
Expand All @@ -7,5 +8,16 @@ export const rootSelector = (state: BaseState): RootState => state.fileData

export const fileFormValues = createSelector(
rootSelector,
state => state.metadataFields
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a bug with this PR where if you change the form fields, do not click UPDATE, and then download the JSON file with the "Export" button, then the file has the "unsaved" name/description not the "saved" one.

It happens because this fileFormValues is also used in file-data/selectors/fileCreator.js's createFile selector, so redirecting it to the unsavedMetadataForm will save the unsaved fields, not the saved fields, in the JSON file.

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'll switch createFile's constituent selector from the unsaved form values to the saved metadata in fileMetadata. And that should fix up the possibility for confusion. The unsaved changes will still exist in the text fields and the update button will still be enabled, which will continue to signal that there are unsaved changes.

state => state.unsavedMetadataForm
)

export const isUnsavedMetadatFormAltered = createSelector(
rootSelector,
state => !_.isEqual(state.unsavedMetadataForm, state.fileMetadata)
)

export const protocolName = createSelector(rootSelector, state => state.fileMetadata.name)
export const fileMetadata = createSelector(
rootSelector,
state => state.fileMetadata
)
4 changes: 2 additions & 2 deletions protocol-designer/src/file-data/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@
import type {DeckSlot, Mount} from '@opentrons/components'
import type {Command} from '../step-generation/types'

export type FilePageFields = {
export type FileMetadataFields = {
name: string,
author: string,
description: string
}

export type FilePageFieldAccessors = $Keys<FilePageFields>
export type FileMetadataFieldAccessors = $Keys<FileMetadataFields>

type MsSinceEpoch = number
type VersionString = string // eg '1.0.0'
Expand Down