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, step-generation): x/Y tip positioning for asp, disp, mix #14758

Merged
merged 8 commits into from
Apr 3, 2024

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Mar 29, 2024

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 and dispense 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

  • refactor the TipPositionField and the modal to take in additional fields
  • wire up the new fields into the form to args components
  • wire it up in step-generation in the pipetting compound commands and then the aspirate and dispense atomic commands
  • fix the tests
  • add a few new unit tests for the TipPositionField and TipPositionModal
  • add new fields to the 8_1_0 migration file
  • update the cypress e2e migration tests
  • create a new tip position modal and remove the hot keys options but add some more details on the visual

Review requests

see test plan

Risk assessment

low

Copy link

codecov bot commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 67.24%. Comparing base (0fbb4c7) to head (8a90c53).
Report is 12 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            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     
Flag Coverage Δ
shared-data 75.93% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...gner/src/components/BatchEditForm/BatchEditMix.tsx 0.00% <ø> (ø)
...c/components/BatchEditForm/BatchEditMoveLiquid.tsx 0.00% <ø> (ø)
...src/components/StepEditForm/fields/DelayFields.tsx 20.00% <ø> (ø)
...nts/StepEditForm/fields/TipPositionField/index.tsx 5.00% <ø> (+1.66%) ⬆️
...ents/StepEditForm/fields/TipPositionField/utils.ts 33.33% <ø> (ø)
...gner/src/components/StepEditForm/forms/MixForm.tsx 0.00% <ø> (ø)
...EditForm/forms/MoveLiquidForm/SourceDestFields.tsx 0.00% <ø> (ø)
protocol-designer/src/form-types.ts 42.85% <ø> (ø)
...r/src/steplist/formLevel/getDefaultsForStepType.ts 81.81% <ø> (ø)
...steplist/formLevel/stepFormToArgs/mixFormToArgs.ts 68.42% <ø> (ø)
... and 11 more

... and 5 files with indirect coverage changes

@jerader jerader marked this pull request as ready for review April 1, 2024 12:10
@jerader jerader requested review from a team as code owners April 1, 2024 12:10
@jerader jerader requested review from TamarZanzouri, a team, jbleon95, Elyorcv, koji and ncdiehl11 and removed request for a team and TamarZanzouri April 1, 2024 12:10
className={styles.pipette_tip_image}
style={{
bottom: `${bottomPx}px`,
left: `calc(${roundedXPx}px - 22px)`,
Copy link
Contributor

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 🤔

Copy link
Collaborator Author

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 %?

Copy link
Contributor

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?

Copy link
Collaborator Author

@jerader jerader Apr 3, 2024

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

Copy link
Contributor

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.

if (isDefault) return errors

const v = Number(value)
if (value === null || Number.isNaN(v)) {
Copy link
Contributor

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.

Copy link
Contributor

@koji koji Apr 1, 2024

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]

Copy link
Collaborator Author

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.

width="5rem"
>
<Icon
name="ot-calibrate"
Copy link
Contributor

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?

Copy link
Collaborator Author

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

"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",
Copy link
Contributor

@koji koji Apr 1, 2024

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"

Copy link
Collaborator Author

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!

@jerader jerader requested a review from koji April 2, 2024 13:09
Copy link
Contributor

@koji koji left a comment

Choose a reason for hiding this comment

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

When I open a modal from Transfer and click custom, z value is empty.
Is this expected behavior or the initial value should be something?

Screenshot 2024-04-03 at 11 56 24 AM

@jerader
Copy link
Collaborator Author

jerader commented Apr 3, 2024

When I open a modal from Transfer and click custom, z value is empty. Is this expected behavior or the initial value should be something?

@koji ya that follows the behavior that already exists for z offset. I was hesitant to change it.

Copy link
Contributor

@koji koji left a comment

Choose a reason for hiding this comment

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

lgtm

@jerader jerader merged commit 3b3c8e4 into edge Apr 3, 2024
39 of 40 checks passed
@jerader jerader deleted the pd_x-y-tip-positioning branch April 3, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants