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

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

Merged
merged 14 commits into from
Jan 30, 2025
4 changes: 2 additions & 2 deletions app/src/assets/localization/en/quick_transfer.json
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
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,12 @@ export function TouchTip(props: TouchTipProps): JSX.Element {
: state.touchTipDispense != null
)
const [currentStep, setCurrentStep] = useState<number>(1)
const [position, setPosition] = useState<number | null>(
kind === 'aspirate'
? state.touchTipAspirate ?? null
: state.touchTipDispense ?? null
const touchTipAspirate =
state.touchTipAspirate != null ? state.touchTipAspirate.toString() : null
const touchTipDispense =
state.touchTipDispense != null ? state.touchTipDispense.toString() : null
const [position, setPosition] = useState<string | null>(
kind === 'aspirate' ? touchTipAspirate : touchTipDispense
)

const touchTipAction =
Expand Down Expand Up @@ -94,7 +96,10 @@ export function TouchTip(props: TouchTipProps): JSX.Element {
setCurrentStep(2)
}
} else if (currentStep === 2) {
dispatch({ type: touchTipAction, position: position ?? undefined })
dispatch({
type: touchTipAction,
position: position != null ? parseInt(position) : undefined,
})
trackEventWithRobotSerial({
name: ANALYTICS_QUICK_TRANSFER_SETTING_SAVED,
properties: {
Expand Down Expand Up @@ -130,10 +135,13 @@ 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)
(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),
Expand Down Expand Up @@ -197,8 +205,8 @@ export function TouchTip(props: TouchTipProps): JSX.Element {
marginTop={SPACING.spacing68}
>
<InputField
type="number"
value={position}
type="text"
value={String(position ?? '')}
title={t('touch_tip_position_mm')}
error={positionError}
readOnly
Expand All @@ -211,10 +219,11 @@ 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

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.

setPosition(Number(e))
setPosition(e)
}}
/>
</Flex>
Expand Down
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
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
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
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,11 @@ 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',
value: null,
type: 'text',
value: '',
},
{}
)
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,
type: 'text',
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,
type: 'text',
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,
type: 'text',
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,
type: 'text',
value: '-8',
},
{}
)
Expand Down
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
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
Loading
Loading