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.

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): update transfer form design #3221

Merged
merged 12 commits into from
Mar 20, 2019

Conversation

IanLondon
Copy link
Contributor

@IanLondon IanLondon commented Mar 15, 2019

overview

Closes #3140

This messes up Mix form, which will be updated in a follow-up PR. However, it should not break anything other than Mix form, including new protocol form, pipette setup, liquids tab, and adding/deleting ingredients in Starting Deck State. (Don't worry about Pause form either, that will be updated right after Mix is!)

This affects Components Library. I addressed our TODOs about replacing inline text with i18n data, and I didn't want to keep i18n responsible for capitalization of labels, so I changed the CSS for FormGroup to capitalize and to append a colon eg 'some label' --> 'Some Label:'.

To opt out of this default capitalization to labels of InputField, CheckboxField, and RadioGroup, you can pass the prop noCapitalize. I added this to all the places where it seemed relevant to Run App. This doesn't append a colon. Nope

I think now, Run App is only visibly affected in one way in the one place it uses FormGroup: now colons are added to the labels in Pipette Config.

image

changelog

  • update Transfer form design
  • expand use of i18n in PD forms
  • add colon and capitalization by default for form component labels via CSS

review requests

  • Design review: Transfer form should meet criteria in PD Steps Form Update: Transfer Form #3140
  • Design question: are new colons in the Pipette Config labels OK? Eg now it says "Plunger Positions:"
  • No regressions in Transfer form functionality (including tooltips and other interactivity)
  • No regressions in other parts of PD
  • No regressions in Run App

Please ignore Mix and Pause forms for now!

@IanLondon IanLondon added app Affects the `app` project ready for review protocol designer Affects the `protocol-designer` project components Affects the `components` project labels Mar 15, 2019
@IanLondon IanLondon requested review from mcous, Kadee80 and b-cooper March 15, 2019 19:03
@codecov
Copy link

codecov bot commented Mar 15, 2019

Codecov Report

Merging #3221 into edge will decrease coverage by 0.05%.
The diff coverage is 5.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #3221      +/-   ##
==========================================
- Coverage   53.77%   53.72%   -0.06%     
==========================================
  Files         698      699       +1     
  Lines       20455    20476      +21     
==========================================
  Hits        11000    11000              
- Misses       9455     9476      +21
Impacted Files Coverage Δ
...er/src/components/StepEditForm/fields/Path/Path.js 0% <ø> (ø) ⬆️
...r/src/components/modals/FilePipettesModal/index.js 0% <ø> (ø) ⬆️
app/src/components/ConfigurePipette/ConfigForm.js 0% <ø> (ø) ⬆️
...-designer/src/components/StepEditForm/ButtonRow.js 0% <ø> (ø) ⬆️
components/src/forms/RadioGroup.js 100% <ø> (ø) ⬆️
components/src/forms/InputField.js 85.71% <ø> (ø) ⬆️
.../components/StepEditForm/fields/BlowoutLocation.js 0% <ø> (ø) ⬆️
components/src/forms/FormGroup.js 100% <ø> (ø) ⬆️
protocol-designer/src/components/FilePage.js 0% <ø> (ø) ⬆️
...ts/StepEditForm/fields/WellOrder/WellOrderModal.js 0% <ø> (ø) ⬆️
... and 15 more

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 ed03ab9...c730f95. Read the comment docs.

Copy link
Contributor

@Kadee80 Kadee80 left a comment

Choose a reason for hiding this comment

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

looks great. i defer to UX on caps/colons but app is unaffected functionally

Copy link
Contributor

@howisthisnamenottakenyet howisthisnamenottakenyet left a comment

Choose a reason for hiding this comment

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

  1. Can the vertical space in the purple bar at the top match the vertical spacing in the purple bar at the bottom?
    image

  2. Remove well order from the top level of aspirate & dispense
    image

  3. Add a really light fill to the advanced settings drawers
    image

@IanLondon IanLondon force-pushed the pd_update-transfer-form-design-3140 branch from f0d9eec to 7638f3b Compare March 20, 2019 14:20
@IanLondon IanLondon requested a review from Kadee80 March 20, 2019 14:20
@@ -26,23 +27,29 @@
justify-content: flex-start;
}

/* TODO: Ian 2019-03-14 delete probably (just used for Mix) */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mix form stuff will be addressed in next PR to update Mix form design

Copy link
Contributor

@Kadee80 Kadee80 left a comment

Choose a reason for hiding this comment

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

No regressions in run app

  • Jog controls radio buttons look the same
  • Pipette config looks the same (except for form group label : )
  • Deck calibration interrupt checkboxes look the same

Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

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

⛺️

@IanLondon IanLondon merged commit 775ec4b into edge Mar 20, 2019
@IanLondon IanLondon deleted the pd_update-transfer-form-design-3140 branch March 20, 2019 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project components Affects the `components` project protocol designer Affects the `protocol-designer` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants