-
Notifications
You must be signed in to change notification settings - Fork 180
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): return tip UI foundation #15823
Conversation
protocol-designer/src/components/StepEditForm/fields/TipWellSelectionField/index.tsx
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipWellSelectionField/index.tsx
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipWellSelectionField/index.tsx
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/forms/MixForm.tsx
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/forms/MoveLiquidForm/index.tsx
Outdated
Show resolved
Hide resolved
migrationModal: null, | ||
unusedPipettes: false, | ||
}, | ||
// TODO(jr, 7/29/24): add these back when we create the 9_0_0 migration for the PD redesign |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: with this PR, importing protocols is a bit wonky because we don't auto-populate the new fields. But i guess since the new fields can be undefined there are no detrimental outcomes with importing.
liquid_selection.mov |
@koji thanks i think this is a bug. i guess youre running into this issue because the labware is incompatible with the pipette (since you are using an 8-channel with a tuberack). I guess we need to disable the well selection if that error comes up. I'll log the bug! |
@@ -35,7 +35,10 @@ export function getDefaultsForStepType( | |||
dispense_delay_seconds: `${DEFAULT_DELAY_SECONDS}`, | |||
mix_touchTip_checkbox: false, | |||
mix_touchTip_mmFromBottom: null, | |||
pickUpTip_location: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shlokamin can i pick your brain about this? so there are 3 new fields introduced with this pr: pickUpTip_location
, pickUpTip_wellNames
, dropTip_wellNames
. Each of them are optional for the user to add so we can create the blank form with each of these values as undefined
. That means that we don't break the import/export (which if we did, then we'd need a new migration and im unsure how we can put all of that behind a ff). But that does not follow the current pattern. All the other fields default to null or an empty array, etc. Is there harm in defaulting to undefined
?
436b826
to
646a11e
Compare
1d8e9cd
to
87b80b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the changes look good to me.
the sandbox worked as expected.
there is one e2e test error on transferSettings.cy.js
@koji thanks! ya the transferSettings cypress test is really brittle - the check tends to fail randomly sometimes. I'll rerun it and it should work hopefully |
closes AUTH-580 AUTH-581 AUTH-582
Overview
Decided to tackle this since I wasn't quite sure how to make the UI (since design doesn't have bandwidth rn to add this feature) so i was messing around and figured i might as well open a PR.
What this PR does is
dropTipField
is modified,pickUpTipField
, and then the well selection input field for both.Please note that there is no error handling and the new fields are not wired up in step-generation.
Test Plan and Hands on Testing
stepForms
->savedStepForms
-> click on the step after the initial deck setup -> navigate to the relevant fields which arepickUpTip_location
,dropTip_location
,pickUpTip_wellNames
anddropTip_wellNames
(note that you might need to expand the tree to see those keys!) See that they populate correctly with the locations showing the tiprack id and the wellnames showing astring[]
of each well nameChangelog
moveLiquid
andmix
Review requests
see test plan
Risk assessment
low, behind ff