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.

Sign up for GitHub

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

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

Merged
merged 14 commits into from
Jan 30, 2025
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4f10226
add support
jerader Jan 28, 2025
b59ebe9
refactor(step-generation, protocol-designer): touchTip to emit from t…
jerader Jan 29, 2025
11ed60a
fix prettier and quick transfer
jerader Jan 29, 2025
9718219
update quick transfer to include negative num pad
jerader Jan 29, 2025
e70cb18
remove console.log
jerader Jan 29, 2025
c6f5f64
address feedback
jerader Jan 29, 2025
c5535c9
fix test
jerader Jan 29, 2025
5093290
fix quick transfer tests
jerader Jan 29, 2025
b015c16
allow negatives in inputField by converting to string
jerader Jan 30, 2025
379a047
fix inputField type
jerader Jan 30, 2025
d6cbf45
make input field type text
jerader Jan 30, 2025
40ac284
fix TouchTip test
jerader Jan 30, 2025
cd2790a
specify lodash floor to 1 decimal
jerader Jan 30, 2025
debd28d
fix cypress e2e test
jerader Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/src/assets/localization/en/quick_transfer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,10 @@
"touch_tip": "Touch tip",
"touch_tip_after_aspirating": "Touch tip after aspirating",
"touch_tip_before_dispensing": "Touch tip before dispensing",
"touch_tip_position_mm": "Touch tip position from bottom of well (mm)",
"touch_tip_position_mm": "Touch tip position from top of well (mm)",
"touch_tip_value": "{{position}} mm from bottom",
"use_deck_slots": "<block>Quick transfers use deck slots B2-D2. These slots hold a tip rack, a source labware, and a destination labware.</block><block>Make sure that your deck configuration is up to date to avoid collisions.</block>",
"value_out_of_range": "Value must be between {{min}}-{{max}}",
"value_out_of_range": "Value must be between {{min}} to {{max}}",
"too_many_pins_body": "Remove a quick transfer in order to add more transfers to your pinned list.",
"too_many_pins_header": "You've hit your max!",
"transfer_analysis_failed": "quick transfer analysis failed",
Expand Down
3 changes: 2 additions & 1 deletion app/src/organisms/ODD/QuickTransferFlow/QuickTransferAdvancedSettings/TouchTip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export function TouchTip(props: TouchTipProps): JSX.Element {
}

// the allowed range for touch tip is half the height of the well to 1x the height
const positionRange = { min: Math.round(wellHeight / 2), max: wellHeight }
const positionRange = { min: -Math.round(wellHeight / 2), max: 0 }
const positionError =
position !== null &&
(position < positionRange.min || position > positionRange.max)
Expand Down Expand Up @@ -211,6 +211,7 @@ export function TouchTip(props: TouchTipProps): JSX.Element {
borderRadius="0"
>
<NumericalKeyboard
hasHyphen
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need an input handler because you can enter hypens in invalid places
Screenshot 2025-01-29 at 1 10 11 PM

Copy link
Collaborator Author

@jerader jerader Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

keyboardRef={keyboardRef}
initialValue={String(position ?? '')}
onChange={e => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Expand Down
8 changes: 4 additions & 4 deletions ...c/organisms/ODD/QuickTransferFlow/__tests__/QuickTransferAdvancedSettings/AirGap.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ describe('AirGap', () => {
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Air gap volume (µL)',
error: 'Value must be between 1-180',
error: 'Value must be between 1 to 180',
readOnly: true,
type: 'number',
value: 0,
Expand Down Expand Up @@ -152,7 +152,7 @@ describe('AirGap', () => {
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Air gap volume (µL)',
error: 'Value must be between 1-80',
error: 'Value must be between 1 to 80',
readOnly: true,
type: 'number',
value: 0,
Expand All @@ -179,7 +179,7 @@ describe('AirGap', () => {
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Air gap volume (µL)',
error: 'Value must be between 1-140',
error: 'Value must be between 1 to 140',
readOnly: true,
type: 'number',
value: 0,
Expand All @@ -204,7 +204,7 @@ describe('AirGap', () => {
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Air gap volume (µL)',
error: 'Value must be between 1-200',
error: 'Value must be between 1 to 200',
readOnly: true,
type: 'number',
value: 0,
Expand Down
6 changes: 3 additions & 3 deletions ...rc/organisms/ODD/QuickTransferFlow/__tests__/QuickTransferAdvancedSettings/Delay.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ describe('Delay', () => {
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Delay duration (seconds)',
error: 'Value must be between 1-9999999999',
error: 'Value must be between 1 to 9999999999',
readOnly: true,
type: 'number',
value: 0,
Expand All @@ -158,7 +158,7 @@ describe('Delay', () => {
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Delay position from bottom of well (mm)',
error: 'Value must be between 1-100',
error: 'Value must be between 1 to 100',
readOnly: true,
type: 'number',
value: 0,
Expand Down Expand Up @@ -188,7 +188,7 @@ describe('Delay', () => {
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Delay position from bottom of well (mm)',
error: 'Value must be between 1-400',
error: 'Value must be between 1 to 400',
readOnly: true,
type: 'number',
value: 0,
Expand Down
2 changes: 1 addition & 1 deletion ...organisms/ODD/QuickTransferFlow/__tests__/QuickTransferAdvancedSettings/FlowRate.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ describe('FlowRate', () => {
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Aspirate flow rate (µL/s)',
error: 'Value must be between 1-92',
error: 'Value must be between 1 to 92',
readOnly: true,
type: 'number',
value: 0,
Expand Down
4 changes: 2 additions & 2 deletions app/src/organisms/ODD/QuickTransferFlow/__tests__/QuickTransferAdvancedSettings/Mix.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ describe('Mix', () => {
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Mix volume (µL)',
error: 'Value must be between 1-200',
error: 'Value must be between 1 to 200',
readOnly: true,
type: 'number',
value: 0,
Expand All @@ -158,7 +158,7 @@ describe('Mix', () => {
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Mix repetitions',
error: 'Value must be between 1-999',
error: 'Value must be between 1 to 999',
readOnly: true,
type: 'number',
value: 0,
Expand Down
2 changes: 1 addition & 1 deletion ...anisms/ODD/QuickTransferFlow/__tests__/QuickTransferAdvancedSettings/PipettePath.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ describe('PipettePath', () => {
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Disposal volume (µL)',
error: 'Value must be between 1-160',
error: 'Value must be between 1 to 160',
readOnly: true,
type: 'number',
value: 201,
Expand Down
4 changes: 2 additions & 2 deletions ...anisms/ODD/QuickTransferFlow/__tests__/QuickTransferAdvancedSettings/TipPosition.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ describe('TipPosition', () => {
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Distance from bottom of well (mm)',
error: 'Value must be between 1-100',
error: 'Value must be between 1 to 100',
readOnly: true,
type: 'text',
value: 0,
Expand All @@ -153,7 +153,7 @@ describe('TipPosition', () => {
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Distance from bottom of well (mm)',
error: 'Value must be between 1-400',
error: 'Value must be between 1 to 400',
readOnly: true,
type: 'text',
value: 0,
Expand Down
36 changes: 20 additions & 16 deletions ...organisms/ODD/QuickTransferFlow/__tests__/QuickTransferAdvancedSettings/TouchTip.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ describe('TouchTip', () => {
fireEvent.click(continueBtn)
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Touch tip position from bottom of well (mm)',
title: 'Touch tip position from top of well (mm)',
error: null,
readOnly: true,
type: 'number',
Expand All @@ -136,15 +136,19 @@ describe('TouchTip', () => {
fireEvent.click(enabledBtn)
const continueBtn = screen.getByText('Continue')
fireEvent.click(continueBtn)
const numButton = screen.getByText('0')
const negButton = screen.getByText('-')
fireEvent.click(negButton)
const numButton = screen.getByText('9')
fireEvent.click(numButton)
const secondNumButton = screen.getByText('8')
fireEvent.click(secondNumButton)
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Touch tip position from bottom of well (mm)',
error: 'Value must be between 25-50',
title: 'Touch tip position from top of well (mm)',
error: 'Value must be between -25 to 0',
readOnly: true,
type: 'number',
value: 0,
value: -98,
},
{}
)
Expand All @@ -162,15 +166,15 @@ describe('TouchTip', () => {
fireEvent.click(enabledBtn)
const continueBtn = screen.getByText('Continue')
fireEvent.click(continueBtn)
const numButton = screen.getByText('0')
const numButton = screen.getByText('1')
fireEvent.click(numButton)
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Touch tip position from bottom of well (mm)',
error: 'Value must be between 100-200',
title: 'Touch tip position from top of well (mm)',
error: 'Value must be between -100 to 0',
readOnly: true,
type: 'number',
value: 0,
value: 1,
},
{}
)
Expand All @@ -184,7 +188,7 @@ describe('TouchTip', () => {
fireEvent.click(enabledBtn)
const continueBtn = screen.getByText('Continue')
fireEvent.click(continueBtn)
const numButton = screen.getByText('4')
const numButton = screen.getByText('0')
fireEvent.click(numButton)
fireEvent.click(numButton)
const saveBtn = screen.getByText('Save')
Expand All @@ -198,19 +202,19 @@ describe('TouchTip', () => {
...props,
state: {
...props.state,
touchTipAspirate: 32,
touchTipAspirate: -25,
},
}
render(props)
const continueBtn = screen.getByText('Continue')
fireEvent.click(continueBtn)
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Touch tip position from bottom of well (mm)',
title: 'Touch tip position from top of well (mm)',
error: null,
readOnly: true,
type: 'number',
value: 32,
value: -25,
},
{}
)
Expand All @@ -222,19 +226,19 @@ describe('TouchTip', () => {
kind: 'dispense',
state: {
...props.state,
touchTipDispense: 118,
touchTipDispense: -8,
},
}
render(props)
const continueBtn = screen.getByText('Continue')
fireEvent.click(continueBtn)
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Touch tip position from bottom of well (mm)',
title: 'Touch tip position from top of well (mm)',
error: null,
readOnly: true,
type: 'number',
value: 118,
value: -8,
},
{}
)
Expand Down
2 changes: 1 addition & 1 deletion app/src/organisms/ODD/QuickTransferFlow/__tests__/VolumeEntry.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ describe('VolumeEntry', () => {
expect(vi.mocked(InputField)).toHaveBeenCalledWith(
{
title: 'Aspirate volume per well (µL)',
error: 'Value must be between 5-50',
error: 'Value must be between 5 to 50',
readOnly: true,
type: 'text',
value: '90',
Expand Down
21 changes: 8 additions & 13 deletions app/src/organisms/ODD/QuickTransferFlow/utils/generateQuickTransferArgs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import {
orderWells,
getAllDefinitions,
getLabwareDefURI,
getWellsDepth,
getTipTypeFromTipRackDefinition,
TRASH_BIN_ADAPTER_FIXTURE,
WASTE_CHUTE_FIXTURES,
Expand Down Expand Up @@ -322,6 +321,12 @@ export function generateQuickTransferArgs(
if (pipetteEntity.spec.channels === 96) {
nozzles = 'ALL' as NozzleConfigurationStyle
}
const touchTipAfterDispenseOffsetMmFromTop =
quickTransferState.touchTipDispense ?? DEFAULT_MM_TOUCH_TIP_OFFSET_FROM_TOP

const touchTipAfterAspirateOffsetMmFromTop =
quickTransferState.touchTipAspirate ?? DEFAULT_MM_TOUCH_TIP_OFFSET_FROM_TOP

const commonFields = {
pipette: pipetteEntity.id,
volume: quickTransferState.volume,
Expand Down Expand Up @@ -355,19 +360,9 @@ export function generateQuickTransferArgs(
aspirateAirGapVolume: quickTransferState.airGapAspirate ?? null,
dispenseAirGapVolume: quickTransferState.airGapDispense ?? null,
touchTipAfterAspirate: quickTransferState.touchTipAspirate != null,
touchTipAfterAspirateOffsetMmFromBottom:
quickTransferState.touchTipAspirate ??
getWellsDepth(quickTransferState.source, sourceWells) +
DEFAULT_MM_TOUCH_TIP_OFFSET_FROM_TOP,
touchTipAfterAspirateOffsetMmFromTop,
touchTipAfterDispense: quickTransferState.touchTipDispense != null,
touchTipAfterDispenseOffsetMmFromBottom:
quickTransferState.touchTipDispense ??
getWellsDepth(
quickTransferState.destination === 'source'
? quickTransferState.source
: quickTransferState.destination,
destWells
) + DEFAULT_MM_TOUCH_TIP_OFFSET_FROM_TOP,
touchTipAfterDispenseOffsetMmFromTop,
dropTipLocation,
aspirateXOffset: 0,
aspirateYOffset: 0,
Expand Down
16 changes: 8 additions & 8 deletions protocol-designer/cypress/e2e/migrations.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,61 +11,61 @@ describe('Protocol fixtures migrate and match snapshots', () => {

const testCases: MigrateTestCase[] = [
{
title: 'example_1_1_0 (schema 1, PD version 1.1.1) -> PD 8.2.x, schema 8',
title: 'example_1_1_0 (schema 1, PD version 1.1.1) -> PD 8.5.x, schema 8',
importTestFile: TestFilePath.Example_1_1_0,
expectedTestFile: TestFilePath.Example_1_1_0V8,
unusedHardware: true,
migrationModal: 'newLabwareDefs',
},
{
title: 'doItAllV3 (schema 3, PD version 4.0.0) -> PD 8.2.x, schema 8',
title: 'doItAllV3 (schema 3, PD version 4.0.0) -> PD 8.5.x, schema 8',
importTestFile: TestFilePath.DoItAllV3V4,
expectedTestFile: TestFilePath.DoItAllV3MigratedToV8,
unusedHardware: false,
migrationModal: 'v8.1',
},
{
title: 'doItAllV4 (schema 4, PD version 4.0.0) -> PD 8.2.x, schema 8',
title: 'doItAllV4 (schema 4, PD version 4.0.0) -> PD 8.5.x, schema 8',
importTestFile: TestFilePath.DoItAllV4V4,
expectedTestFile: TestFilePath.DoItAllV4MigratedToV8,
unusedHardware: false,
migrationModal: 'v8.1',
},
{
title:
'doItAllv7MigratedToV8 (schema 7, PD version 8.0.0) -> should migrate to 8.2.x, schema 8',
'doItAllv7MigratedToV8 (schema 7, PD version 8.0.0) -> should migrate to 8.5.x, schema 8',
importTestFile: TestFilePath.DoItAllV7,
expectedTestFile: TestFilePath.DoItAllV7MigratedToV8,
unusedHardware: false,
migrationModal: 'v8.1',
},
{
title:
'96-channel full and column schema 8 -> should migrate to 8.2.x, schema 8',
'96-channel full and column schema 8 -> should migrate to 8.5.x, schema 8',
importTestFile: TestFilePath.NinetySixChannelFullAndColumn,
expectedTestFile: TestFilePath.NinetySixChannelFullAndColumn,
unusedHardware: false,
migrationModal: null,
},
{
title:
'doItAllV8 flex robot -> reimported, should migrate to 8.2.x, schema 8',
'doItAllV8 flex robot -> reimported, should migrate to 8.5.x, schema 8',
importTestFile: TestFilePath.DoItAllV8,
expectedTestFile: TestFilePath.DoItAllV8,
unusedHardware: false,
migrationModal: null,
},
{
title:
'new advanced settings with multi temp => reimported, should not migrate and stay at 8.2.x, schema 8',
'new advanced settings with multi temp => reimported, should not migrate and stay at 8.5.x, schema 8',
importTestFile: TestFilePath.NewAdvancedSettingsAndMultiTemp,
expectedTestFile: TestFilePath.NewAdvancedSettingsAndMultiTemp,
unusedHardware: false,
migrationModal: null,
},
{
title:
'thermocycler on Ot2 (schema 7, PD version 7.0.0) -> should migrate to 8.2.x, schema 8',
'thermocycler on Ot2 (schema 7, PD version 7.0.0) -> should migrate to 8.5.x, schema 8',
importTestFile: TestFilePath.ThermocyclerOnOt2V7,
expectedTestFile: TestFilePath.ThermocyclerOnOt2V7MigratedToV8,
migrationModal: 'v8.1',
Expand Down
Loading
Loading