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

fix(protocol-designer): various select pipette bug in onboarding flow #17001

Merged
merged 3 commits into from
Dec 2, 2024

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Dec 2, 2024

closes RQA-3721 RQA-3716

Overview

Fixes some bugs with the 96-channel option not showing up correctly
Fixes the copy for adding a 2nd pipette
fixes the ability to add a 2nd pipette but then proceed without completing

Test Plan and Hands on Testing

Test adding/removing/editing pipettes in the select pipette portion of the onboarding flow.

Changelog

  • various bug fixes and refining the logic

Risk assessment

low

@jerader jerader requested a review from a team as a code owner December 2, 2024 14:28
@jerader
Copy link
Collaborator Author

jerader commented Dec 2, 2024

  • double check the logic for editing the pipettes from the overview page, i think the logic is the same there so make sure it works as expected

@jerader jerader requested review from koji and ncdiehl11 December 2, 2024 14:29
@jerader
Copy link
Collaborator Author

jerader commented Dec 2, 2024

  • double check the logic for editing the pipettes from the overview page, i think the logic is the same there so make sure it works as expected

looks like the editing the instruments on the overview page works a bit differently and logic seems to be working to me ⭐

@koji
Copy link
Contributor

koji commented Dec 2, 2024

the back button works but when removing 2 pipettes, confirm button is clickable.

steps

  1. select the first pipette
  2. select the second pipette
  3. remove all of them
Screenshot 2024-12-02 at 11 03 09 AM

Comment on lines 116 to 118
page === 'add' &&
pipettesByMount[defaultMount].tiprackDefURI == null &&
noPipette
Copy link
Contributor

Choose a reason for hiding this comment

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

for the attached screenshot case

Suggested change
page === 'add' &&
pipettesByMount[defaultMount].tiprackDefURI == null &&
noPipette
const isDisabled =
(page === 'add' &&
pipettesByMount[defaultMount].tiprackDefURI == null &&
noPipette) ||
selectedValues.length === 0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this results in requiring to add a 2nd pipette when you go back to add a 2nd pipette. let me look into another option

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok i think i found a way that works!

@jerader jerader requested a review from koji December 2, 2024 16:52
Copy link
Collaborator

@ncdiehl11 ncdiehl11 left a comment

Choose a reason for hiding this comment

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

couple nits, but looks good

(page === 'add' &&
pipettesByMount[defaultMount].tiprackDefURI == null &&
noPipette) ||
(pipettesByMount.left.tiprackDefURI == null &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit, maybe we could add a utility to get these values rather than checking both mounts. not necessary in this PR

@@ -174,6 +166,16 @@ export function SelectPipettes(props: WizardTileProps): JSX.Element | null {
}
}, [location])

const hasAPipette =
(defaultMount === 'left' && pipettesByMount.right.pipetteName != null) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

@@ -76,7 +76,7 @@ export function SelectPipettes(props: WizardTileProps): JSX.Element | null {
const allowAllTipracks = useSelector(getAllowAllTipracks)
const allPipetteOptions = getAllPipetteNames('maxVolume', 'channels')
const robotType = fields.robotType
const defaultMount = mount ?? 'left'
const defaultMount = mount
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no lol i can remove it

@jerader jerader merged commit 2a4a3c6 into edge Dec 2, 2024
14 checks passed
@jerader jerader deleted the pd_select-pip-bugs branch December 2, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants