-
Notifications
You must be signed in to change notification settings - Fork 178
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, step-generation): x/Y tip positioning for asp, disp, mix #14758
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## edge #14758 +/- ##
==========================================
+ Coverage 67.17% 67.24% +0.06%
==========================================
Files 2495 2495
Lines 71483 71352 -131
Branches 9020 8971 -49
==========================================
- Hits 48020 47980 -40
+ Misses 21341 21256 -85
+ Partials 2122 2116 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
52a053e
to
21244f3
Compare
protocol-designer/src/components/StepEditForm/fields/TipPositionField/TipPositionAllViz.tsx
Outdated
Show resolved
Hide resolved
className={styles.pipette_tip_image} | ||
style={{ | ||
bottom: `${bottomPx}px`, | ||
left: `calc(${roundedXPx}px - 22px)`, |
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.
nit
If we could use %
instead of the fixed value, it would be good. I guess the fixed number might be different from a browser's window size 🤔
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.
hmm the fixed number seems to work even with the browser's window size changing. I am not sure how to get the total width to convert to %?
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.
Do we have the design for this in Figma?
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.
no :/ Felix sort of has bandwidth but otherwise we have no designer for this phase lol
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.
okay, I think we don't need to think about this now.
...col-designer/src/components/StepEditForm/fields/TipPositionField/TipPositionInput.module.css
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipPositionField/ZTipPositionModal.tsx
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipPositionField/ZTipPositionModal.tsx
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipPositionField/ZTipPositionModal.tsx
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipPositionField/ZTipPositionModal.tsx
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipPositionField/ZTipPositionModal.tsx
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipPositionField/utils.ts
Outdated
Show resolved
Hide resolved
if (isDefault) return errors | ||
|
||
const v = Number(value) | ||
if (value === null || Number.isNaN(v)) { |
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.
I think value === null
may not work as you expected because Number(null)
returns 0.
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.
maybe
if(value === null) return [OUT_OF_BOUNDS]
const convertedValue = Number(value)
if(Number.isNaN(convertedValue)) return [OUT_OF_BOUNDS]
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.
i think this still works as expected since it is const v that is being turned into a number.
protocol-designer/src/components/StepEditForm/fields/TipPositionField/ZTipPositionModal.tsx
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipPositionField/ZTipPositionModal.tsx
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipPositionField/ZTipPositionModal.tsx
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipPositionField/ZTipPositionModal.tsx
Show resolved
Hide resolved
...gner/src/components/StepEditForm/fields/TipPositionField/__tests__/TipPositionField.test.tsx
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipPositionField/index.tsx
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipPositionField/index.tsx
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipPositionField/index.tsx
Outdated
Show resolved
Hide resolved
width="5rem" | ||
> | ||
<Icon | ||
name="ot-calibrate" |
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.
can we specify the icon size?
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.
size is specified in the classname
protocol-designer/src/components/StepEditForm/fields/TipPositionField/utils.ts
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipPositionField/utils.ts
Outdated
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/fields/TipPositionField/utils.ts
Outdated
Show resolved
Hide resolved
"dispense_touchTip_checkbox": "Touch tip to each side of well after dispensing", | ||
"dispense_touchTip_mmFromBottom": "Distance from the bottom of the well", | ||
"disposalVolume_checkbox": "Aspirate extra volume that is disposed of after a multi-dispense is complete. We recommend a disposal volume of at least the pipette's minimum.", | ||
"heaterShakerSetTimer": "Once this counter has elapsed, the module will deactivate the heater and shaker", | ||
"mix_mmFromBottom": "Distance from the bottom of the well", | ||
"mix_mmFromBottom": "Adjust tip position", |
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.
can we use one item for "Adjust tip position"
?
adjust_tip_position: "Adjust tip position"
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 way the PD tooltip logic works, each form field needs its own tooltip where the key is equal to the form field's name. That results in a lot of repeated text. I will specify the text a bit though!
...col-designer/src/components/StepEditForm/fields/TipPositionField/TipPositionInput.module.css
Show resolved
Hide resolved
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.
@koji ya that follows the behavior that already exists for z offset. I was hesitant to change it. |
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.
lgtm
closes AUTH-5
Overview
Now, when you are aspirating, dispensing or mixing, you have the option of setting your x/y tip position (if it is available aka not in waste chute or trash)
Test Plan
sandbox: https://sandbox.designer.opentrons.com/pd_x-y-tip-positioning/
Create a flex or ot-2 protocol and add a mix step, in the advanced setting, click on the tip positioning icon and look at the modal, try setting custom values. Now do the same thing with a transfer step. Observe the tip position modal and make sure it looks good enough and makes sense (i'll get design to look at it soon and then have a followup DQA ticket) The protocol should export correctly and reimport correctly. In the exported json file, examine the
aspirate
anddispense
commands. Make sure the x,y,z offsets are as you expect them to be. Note: if you did nothing, the defaults for x and y are 0.Changelog
TipPositionField
and the modal to take in additional fieldsTipPositionField
andTipPositionModal
8_1_0
migration fileReview requests
see test plan
Risk assessment
low