refactor(step-generation, protocol-designer, app): touchTip to emit from top of well origin #17375

jerader · 2025-01-29T13:44:12Z

Overview

Previously, touchTip would occur from the bottom of the well, which is a bit confusing since users in python and intuitively would want to start with origin from the top. This PR migrates all touch tip fields and inverses the logic. Here is the summary of the changes:

aspirate_touchTipMmFromBottom is now aspirate_touchTipMmFromTop in moveLiquid form data
dispense_touchTipMmFromBottom is now dispense_touchTipMmFromTop in moveLiquid form data
mix_touchTipMmFromBottom is now mix_touchTipMmFromTop in mix form data
the modal in the form is updated to grab the values starting from the top of the well, going into the well the values are negative. This updated in PD AND Quick transfer
step-generation now emits touchTip to be origin from the top of the well with the new z-index
old protocols now go through a 8_5_0 migration to migrate their form data z-index fields to be origin from the top of the well

Test Plan and Hands on Testing

First test that this makes sense and works in PD by testing a mix's touch tip and move liquid's aspirate and dispense touch tip. See that the modal starts from the top of the well for adjusting the position

Then test importing an old protocol and see that it migrates correclty for those fields, making sure touchtip is now origin at the top of the well and also check the form

Then test that quick transfer's touch tip on the ODD has a min/max that matches this

Changelog

update the touch tip fields and add a 8_5_0 migration file, migrating the cypress e2e tests as well
update step-generation's touch tip atomic command origin
update step args to match the new touch tip field name
fix affected tests
update quick transfer to match this change

Risk assessment

med-ish since this touches a good amount of code. But with unit and cypress tests passing, i'm confident that this change is correctly implemented

ncdiehl11

Left a bunch of nits, mostly pertaining to testing. Only blocking fix that needs to be made that I can tell is the hypen issue in the input field

ncdiehl11 · 2025-01-29T18:10:52Z

app/src/organisms/ODD/QuickTransferFlow/QuickTransferAdvancedSettings/TouchTip.tsx

@@ -211,6 +211,7 @@ export function TouchTip(props: TouchTipProps): JSX.Element {
            borderRadius="0"
          >
            <NumericalKeyboard
+              hasHyphen


I think we need an input handler because you can enter hypens in invalid places

is this modal from PD? I'm a bit confused because you commented on the code that is part of TouchTip from Quick Transfer

sorry, i left this comment in the wrong place. this is PD

thanks, i think i'll leave as is and address that in a follow up. seems like it is an issue for blowOut and touchTip

protocol-designer/src/organisms/TipPositionModal/utils.tsx

protocol-designer/src/steplist/formLevel/stepFormToArgs/mixFormToArgs.ts

protocol-designer/src/steplist/formLevel/stepFormToArgs/moveLiquidFormToArgs.ts

protocol-designer/src/ui/steps/test/selectors.test.ts

step-generation/src/__tests__/consolidate.test.ts

step-generation/src/__tests__/distribute.test.ts

step-generation/src/__tests__/transfer.test.ts

step-generation/src/fixtures/commandFixtures.ts

...rc/organisms/ODD/QuickTransferFlow/__tests__/QuickTransferAdvancedSettings/TouchTip.test.tsx

protocol-designer/fixtures/protocol/8/doItAllV3MigratedToV8.json

ddcc4 · 2025-01-29T19:20:12Z

protocol-designer/src/load-file/migration/8_5_0.ts

@@ -0,0 +1,162 @@
+import floor from 'lodash/floor'


Hm. What's the reason for lodash floor instead of the built-in floor function?

good call, i can probably just use Math.floor()

oh lodash floor allows you to specify the decimal points. math.floor() does not. so i'll keep the lodash floor

That's fine, but I meant: where in the code do you actually specify the places after the decimal then if that's what you want?

we specify the places in the decimal at the form input field level. like the input field restricts users to 1 decimal. I suppose we could convert it upstream at that level. But it seems cleaner to me to convert it here in the migration. What do you think?

Hm, I think I'm confused: In the code, you added stuff like

floor( dispense_touchTip_mmFromBottom - matchingDispenseLabwareWellDepth ),

I don't see where the rounding to 1 place after the decimal happens. Does lodash floor() default to 1 place? The built-in Math.floor() would just truncate it to an integer.

oops i forgot to add back the decimal value. it should be this. thank you for catching this. I had it right initially but then didn't fix it when i reverted the math.floor change.

floor( dispense_touchTip_mmFromBottom - matchingDispenseLabwareWellDepth, 1 )

Ah, gotcha!
(I'm curious if changing it to floor(..., 1) should change the expected output in any of the tests.)

protocol-designer/src/load-file/migration/8_5_0.ts

…op of well origin closes AUTH-1378

smb2268

As is currently, you aren't able to enter the - on the quick transfer touch tip screen because of typing expectations with the input field. I played around with it and think this suggestion fixes the issue, but let me know if you have questions!

smb2268 · 2025-01-29T22:46:18Z

app/src/organisms/ODD/QuickTransferFlow/QuickTransferAdvancedSettings/TouchTip.tsx

@@ -211,6 +211,7 @@ export function TouchTip(props: TouchTipProps): JSX.Element {
            borderRadius="0"
          >
            <NumericalKeyboard
+              hasHyphen
              keyboardRef={keyboardRef}
              initialValue={String(position ?? '')}
              onChange={e => {


With the addition of the -, this typing is no longer accurate. I think the type for InputField above has to change to text and this should be setPosition(e) without the cast to Number. We'll then have to update positionError to something like this:

const positionError = position !== null && (position === '-' || position.indexOf('-') !== position.lastIndexOf('-') || Number(position) < positionRange.min || Number(position) > positionRange.max) ? t(`value_out_of_range`, { min: positionRange.min, max: Math.floor(positionRange.max), }) : null

and then handle casting from number to string when grabbing the initial values from state and then from string to number sending them via dispatch

Thanks for catching this Sarah! I just pushed a fix according to your comment. Hopefully it works as expected now.

ddcc4

OK, approving. I don't have much context for the Quick Transfer and ODD stuff, so hopefully someone else has looked at those changes.

ddcc4 · 2025-01-30T16:54:53Z

protocol-designer/src/load-file/migration/8_5_0.ts

@@ -0,0 +1,162 @@
+import floor from 'lodash/floor'


Ah, gotcha!
(I'm curious if changing it to floor(..., 1) should change the expected output in any of the tests.)

smb2268

Tested out the quick transfer changes and they're now working as expected! Thanks for updating

jerader changed the title ~~pd_touch-tip-migrate~~ refactor(step-generation, protocol-designer): touchTip to emit from top of well origin Jan 29, 2025

jerader changed the title ~~refactor(step-generation, protocol-designer): touchTip to emit from top of well origin~~ refactor(step-generation, protocol-designer, app): touchTip to emit from top of well origin Jan 29, 2025

jerader mentioned this pull request Jan 29, 2025

feat(protocol-designer): add submerge for aspirate and dispense #17368

Merged

jerader marked this pull request as ready for review January 29, 2025 15:36

jerader requested review from a team as code owners January 29, 2025 15:36

jerader requested review from mjhuff, koji, smb2268, ddcc4, ncdiehl11 and syao1226 and removed request for a team January 29, 2025 15:36

ncdiehl11 reviewed Jan 29, 2025

View reviewed changes

ddcc4 reviewed Jan 29, 2025

View reviewed changes

jerader requested review from ddcc4 and ncdiehl11 January 29, 2025 20:59

jerader added 7 commits January 29, 2025 16:49

add support

4f10226

refactor(step-generation, protocol-designer): touchTip to emit from t…

b59ebe9

…op of well origin closes AUTH-1378

fix prettier and quick transfer

11ed60a

update quick transfer to include negative num pad

9718219

remove console.log

e70cb18

address feedback

c6f5f64

fix test

c5535c9

jerader force-pushed the pd_touch-tip-migrate branch from ffd9f2b to c5535c9 Compare January 29, 2025 21:50

fix quick transfer tests

5093290

smb2268 requested changes Jan 29, 2025

View reviewed changes

allow negatives in inputField by converting to string

b015c16

jerader requested a review from smb2268 January 30, 2025 14:07

jerader added 4 commits January 30, 2025 11:03

fix inputField type

379a047

make input field type text

d6cbf45

fix TouchTip test

40ac284

specify lodash floor to 1 decimal

cd2790a

ddcc4 approved these changes Jan 30, 2025

View reviewed changes

smb2268 approved these changes Jan 30, 2025

View reviewed changes

fix cypress e2e test

debd28d

jerader merged commit b1684cb into edge Jan 30, 2025
27 checks passed

jerader deleted the pd_touch-tip-migrate branch January 30, 2025 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(step-generation, protocol-designer, app): touchTip to emit from top of well origin #17375

refactor(step-generation, protocol-designer, app): touchTip to emit from top of well origin #17375

jerader commented Jan 29, 2025

ncdiehl11 left a comment

ncdiehl11 Jan 29, 2025

jerader Jan 29, 2025 •

edited

Loading

ncdiehl11 Jan 29, 2025

jerader Jan 29, 2025

ddcc4 Jan 29, 2025

jerader Jan 29, 2025

jerader Jan 29, 2025

ddcc4 Jan 29, 2025

jerader Jan 30, 2025

ddcc4 Jan 30, 2025

jerader Jan 30, 2025

ddcc4 Jan 30, 2025

smb2268 left a comment

smb2268 Jan 29, 2025

jerader Jan 30, 2025

ddcc4 left a comment

ddcc4 Jan 30, 2025

smb2268 left a comment

refactor(step-generation, protocol-designer, app): touchTip to emit from top of well origin #17375

refactor(step-generation, protocol-designer, app): touchTip to emit from top of well origin #17375

Conversation

jerader commented Jan 29, 2025

Overview

Test Plan and Hands on Testing

Changelog

Risk assessment

ncdiehl11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerader Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smb2268 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddcc4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smb2268 left a comment

Choose a reason for hiding this comment

jerader Jan 29, 2025 •

edited

Loading