Pd file detail form edit #1673

b-cooper · 2018-06-08T21:40:14Z

overview

changelog

This Adds a new render prop component (HOC) to the component library. It is called KeypressHandler and will allow us to consolidate keyCode mappings and keypress logic to a single source going forward. It is currently being used in the File Metadata form in order to allow for submit/save on enter key down.

review requests

Try out creating a protocol and editing any and all of its field from the File Details page.

The Protocol Name in the title bar should update when it's value is updated in the form.
The submit button should be disabled unless the values within the form have changed.

Closes #1504 and Closes #1661

howisthisnamenottakenyet · 2018-06-11T13:42:23Z

@b-cooper Can't seem to get the amazon link working for this one.

IanLondon

Left a buncha comments but to summarize:

Travis is failing b/c of flow errors https://travis-ci.org/Opentrons/opentrons/builds/389928470#L1360
Bug where file is saved with "unsaved" fields
HOC seems like a harder way to use onKeyDown, and for this use case it seems easier to use a form and its onSubmit prop

IanLondon · 2018-06-11T13:33:41Z

components/src/interaction-enhancers/KeypressHandler.js

+import * as React from 'react'
+import _ from 'lodash'
+
+const KEY_CODES = {


keyCode is deprecated: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode -- now the standard is key https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key

But also it seems like the purpose of this whole enhancer is to make a new alternative mini-API for handling key press events -- instead of being able to have full control over the onKeyDown, you give up control (eg control over allowing event bubbling, and over handling key modifiers like ctrl or shift) for syntax shortcut. I can't think of how this would have a broad use case. IMHO it seems more straightforward to use onKeyDown prop directly and check the key in whatever callback you use.

That's fair. I'll use the event.key directly in the onKeyDown prop going forward.

IanLondon · 2018-06-11T13:58:06Z

protocol-designer/src/components/FilePage.js

+      <h2>
+        Information
+      </h2>
+      <KeypressHandler


Could this be a <form> with onSubmit={saveFileMetaData} (which should also give you the save on Enter key behavior) and avoid the onKeyDown handling?

IanLondon · 2018-06-11T17:43:30Z

protocol-designer/src/file-data/selectors/fileFields.js

@@ -7,5 +8,12 @@ export const rootSelector = (state: BaseState): RootState => state.fileData

 export const fileFormValues = createSelector(
  rootSelector,
-  state => state.metadataFields


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.

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.

howisthisnamenottakenyet · 2018-06-11T18:20:04Z

@IanLondon I would say we might as well leave the unsaved changes in text fields until we get feedback that changes our mind.

codecov · 2018-06-11T20:13:23Z

Codecov Report

Merging #1673 into edge will decrease coverage by <.01%.
The diff coverage is 20.58%.

@@            Coverage Diff             @@
##             edge    #1673      +/-   ##
==========================================
- Coverage   34.68%   34.67%   -0.01%     
==========================================
  Files         342      342              
  Lines        5576     5592      +16     
==========================================
+ Hits         1934     1939       +5     
- Misses       3642     3653      +11

Impacted Files	Coverage Δ
...tocol-designer/src/containers/ConnectedFilePage.js	`0% <0%> (ø)`	⬆️
...ol-designer/src/containers/ConnectedFileSidebar.js	`0% <0%> (ø)`	⬆️
...tocol-designer/src/containers/ConnectedTitleBar.js	`0% <0%> (ø)`	⬆️
...l-designer/src/containers/ConnectedNewFileModal.js	`0% <0%> (ø)`	⬆️
...ol-designer/src/file-data/selectors/fileCreator.js	`20% <0%> (ø)`	⬆️
protocol-designer/src/components/FilePage.js	`0% <0%> (ø)`	⬆️
protocol-designer/src/file-data/actions.js	`100% <100%> (ø)`	⬆️
protocol-designer/src/file-data/reducers/index.js	`40% <40%> (-1.67%)`	⬇️
...col-designer/src/file-data/selectors/fileFields.js	`62.5% <50%> (-4.17%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96c6b3c...28af4f0. Read the comment docs.

howisthisnamenottakenyet · 2018-06-11T20:16:32Z

Nitpicky but: Can the text not read "saved"? Because it's on the file details tab I want to be super clear that this isn't stored in your browser yet. Updated would be preferred for now.

IanLondon

almost!

IanLondon · 2018-06-11T21:25:24Z

protocol-designer/src/components/FilePage.js

+      <h2>
+        Information
+      </h2>
+      <form onSubmit={() => { console.log('DID IT') }}>


If you do <form onSubmit={saveFileMetadata}> and remove the onClick of the submit button, the form's onSubmit will fire. I'm not sure what it means to have an onClick on a type=submit button... but since it's not logging 'DID IT', the onSubmit is getting ~~blocked~~ overridden by the onClick somehow. (As the code is now, the button's onClick fires when you hit enter, which we want to happen, but weird that the button's onClick is fired when you hit enter and the form's onSubmit is not...)

I tried locally removing the onClick and the 'DID IT' prints

IanLondon · 2018-06-11T21:25:57Z

protocol-designer/src/components/FilePage.js

-      </section>
+        <div className={styles.button_row}>
+          <OutlineButton type="submit" className={styles.update_button} onClick={saveFileMetadata} disabled={!isFormAltered}>
+            {isFormAltered ? 'UPDATE' : 'SAVED'}


Putting a comment here so I don't forget - Morgan requested 'UPDATED' instead of 'SAVED'

IanLondon

🍽️ sweeet

approved! (pending CI)

mcous

Got a couple things that should be addressed, but fine to grab them in the next PR (or whenever):

Stale TODO comment (I think)
Don't import all of lodash

Other comments are just for discussion

mcous · 2018-06-11T22:20:11Z

protocol-designer/src/components/FilePage.js

 }

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


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

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

mcous · 2018-06-11T22:32:01Z

protocol-designer/src/components/FilePage.js

-export default function FilePage (props: Props) {
-  const {formConnector, instruments} = props
+const FilePage = ({formConnector, isFormAltered, instruments, saveFileMetadata}: FilePageProps) => {
+  const handleSubmit = () => {


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)

mcous · 2018-06-11T22:40:55Z

protocol-designer/src/containers/ConnectedTitleBar.js

 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


Should this TODO comment be removed?

mcous · 2018-06-11T22:42:53Z

protocol-designer/src/file-data/selectors/fileFields.js

@@ -1,4 +1,5 @@
 // @flow
+import _ from 'lodash'


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

import isEqual from 'lodash/isEqual'

good catch!

Make file metadata form clearer about when you're altering the saved information and when you can update it by submitting. Also added in key press handler render prop component to the component library (used here for handling submission on pressing the enter key). Closes #1504 and #1661

b-cooper requested review from IanLondon, mcous and howisthisnamenottakenyet June 8, 2018 21:40

b-cooper added feature Ticket is a feature request / PR introduces a feature protocol designer Affects the `protocol-designer` project small labels Jun 8, 2018

IanLondon suggested changes Jun 11, 2018

View reviewed changes

b-cooper added the ready for review label Jun 11, 2018

IanLondon suggested changes Jun 11, 2018

View reviewed changes

IanLondon approved these changes Jun 11, 2018

View reviewed changes

mcous approved these changes Jun 11, 2018

View reviewed changes

howisthisnamenottakenyet approved these changes Jun 12, 2018

View reviewed changes

b-cooper added 9 commits June 15, 2018 11:04

get a button and some more selectors out there

c57872f

wire up the save

db9d6a2

move towards correct file name

de4b90a

enter key press handler

91b684d

onKeyDown flow type

a3c4e57

[fix-up] address ian pr comments

4955d20

[fixup] morgan nit and ian comment addressed

7c83585

remove stale TODO

28af4f0

b-cooper force-pushed the pd_file-detail-form-edit branch from 71e29c4 to 28af4f0 Compare June 15, 2018 15:14

b-cooper merged commit d03d42f into edge Jun 15, 2018

mcous deleted the pd_file-detail-form-edit branch June 28, 2018 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pd file detail form edit #1673

Pd file detail form edit #1673

b-cooper commented Jun 8, 2018 •

edited

Loading

howisthisnamenottakenyet commented Jun 11, 2018

IanLondon left a comment •

edited

Loading

IanLondon Jun 11, 2018

IanLondon Jun 11, 2018

b-cooper Jun 11, 2018 •

edited

Loading

IanLondon Jun 11, 2018

IanLondon Jun 11, 2018

b-cooper Jun 11, 2018

howisthisnamenottakenyet commented Jun 11, 2018

codecov bot commented Jun 11, 2018 •

edited

Loading

howisthisnamenottakenyet commented Jun 11, 2018

IanLondon left a comment

IanLondon Jun 11, 2018 •

edited

Loading

IanLondon Jun 11, 2018

IanLondon left a comment

mcous left a comment

mcous Jun 11, 2018

IanLondon Jun 12, 2018 •

edited

Loading

mcous Jun 11, 2018

mcous Jun 11, 2018

mcous Jun 11, 2018

IanLondon Jun 12, 2018

Pd file detail form edit #1673

Pd file detail form edit #1673

Conversation

b-cooper commented Jun 8, 2018 • edited Loading

overview

changelog

review requests

howisthisnamenottakenyet commented Jun 11, 2018

IanLondon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-cooper Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howisthisnamenottakenyet commented Jun 11, 2018

codecov bot commented Jun 11, 2018 • edited Loading

Codecov Report

howisthisnamenottakenyet commented Jun 11, 2018

IanLondon left a comment

Choose a reason for hiding this comment

IanLondon Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanLondon left a comment

Choose a reason for hiding this comment

mcous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanLondon Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-cooper commented Jun 8, 2018 •

edited

Loading

IanLondon left a comment •

edited

Loading

b-cooper Jun 11, 2018 •

edited

Loading

codecov bot commented Jun 11, 2018 •

edited

Loading

IanLondon Jun 11, 2018 •

edited

Loading

IanLondon Jun 12, 2018 •

edited

Loading