refactor(app,pd): add GEN1 category to OG pipettes, FF'd GEN2 select to PD #4228
Conversation
mcous
added
app
Codecov Report
@@ Coverage Diff @@
## edge #4228 +/- ##
==========================================
- Coverage 56.35% 56.33% -0.02%
==========================================
Files 869 869
Lines 24564 24575 +11
==========================================
+ Hits 13843 13845 +2
- Misses 10721 10730 +9
Continue to review full report at Codecov.
|
| @@ -60,10 +60,6 @@ | |||
| font-size: var(--fs-caption); | |||
| } | |||
|
|
|||
| .pipette_select { | |||
There was a problem hiding this comment.
Double check that this removal doesn't mess with the app's change pipette wizard
There was a problem hiding this comment.
attach/change pipette wizard looks OK to me in RA
| return ( | ||
| <Select | ||
| isSearchable={false} | ||
| className={styles.pipette_select} | ||
| menuPosition="fixed" |
There was a problem hiding this comment.
This change was necessary to stop the dropdown from pushing everything below it down the page. Not sure why that is, but this looks like it's working for now.
We may want to investigate portals as a potential solution here (along with the tooltip stuff that @b-cooper has noticed)
| @@ -41,7 +23,8 @@ export function getPipetteModelSpecs(model: string): ?PipetteModelSpecs { | |||
| const modelSpecificFields = pipetteModelSpecs.config[model] | |||
| const modelFields = | |||
| modelSpecificFields && getPipetteNameSpecs(modelSpecificFields.name) | |||
| return modelFields && { ...modelFields, ...modelSpecificFields } | |||
|
|
|||
| return modelFields && { ...modelFields, ...modelSpecificFields, model } | |||
There was a problem hiding this comment.
The flow type for this return specified that model: string would be returned in this object, but that was not the case. Rather than adjusting the type, I added model to the return object to match how getPipetteNameSpecs adds the name to the returned object
| |} | ||
|
|
||
| // TODO(mc, 2019-10-14): update this type according to the schema | ||
| export type PipetteModelSpecs = { |
There was a problem hiding this comment.
This is an inexact type because there are other fields. Added a TODO rather than making this PR bigger with something that it doesn't strictly need
There was a problem hiding this comment.
PD, Run App, and component library changes look good!
If you try to run these protocols you get: ValidationError: 'p20_single_gen2' is not one of ['p10_single', 'p10_multi', 'p50_single', 'p50_multi', 'p300_single', 'p300_multi', 'p1000_single', 'p1000_multi']
Not a blocker for this PR. Probably we should expand the schema which I generally wouldn't want to do but here it makes sense. Maybe even to string instead of enum... If we do that we ought to guard against PD importing protocols with pipettes it doesn't understand, and guard against JSON v3 executor running protocols with pipettes it doesn't understand, with user-readable errors in both cases. Unfortunately for past & existing versions of PD/API we can't make the error nicer after the fact.
| @@ -60,10 +60,6 @@ | |||
| font-size: var(--fs-caption); | |||
| } | |||
|
|
|||
| .pipette_select { | |||
There was a problem hiding this comment.
attach/change pipette wizard looks OK to me in RA
overview
This PR adds the new pipette select component from #3996 and adds it to PD behind a feature flag.
changelog
review requests
Due to lack specs/screens, I had to make the following judgement calls:
displayCategoryfor original pipettes ofGEN1was added to the pipette config and UI as per various ad-hoc Slack conversations and PD: Add generations of pipettes to Protocol Designer #4138displayNamedisplayNameproperties do not include "GEN1"testing
App:
__DEV__ enablePipettePlusPD:
Enable GEN2 pipettessetting