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): correct step count in create file wizard #13807

Merged
merged 4 commits into from
Oct 19, 2023

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Oct 18, 2023

closes RAUT-775

Overview

Now that a staging area slot tile was created for the Flex, the step count at the top of the create file wizard was incorrect for the Ot-2. This PR fixes that as well as fixing the go back button from the last page of the wizard.

NOTE: the total step counts stay as 7 for Flex and 6 for OT-2 regardless of if you end up skipping steps (if you don't add a 2nd pipette or if you add the 96-channel). I opted for this because a user will be aware of the total steps possible for the certain robot. Please let me know if you have any questions/concerns about this though!

Test Plan

  • turn on the deck modification & 96-channel feature flags. Create a Flex protocol and go thorugh the file wizard, make sure the step count work as expected for a total of 7 steps. See that if you add a 96-channel or don't add a 2nd pipette that the wizard skips to the appropriate page but the total step count remains at 7. Try to go back on the Additional Items (last page) and see that it goes to the appropriate page depending on if you have a 2nd pipette or not.

  • now follow those steps for an Ot-2 protocol. See that the total number of steps is 6 and that the Staging Area tile is skipped. Try the variations of adding a 2nd pipette/not adding a 2nd pipette. Make sure the step counts work as expected.

  • now turn off the deck modification feature flag and create a Flex protocol, see that it skips the Staging Area tile.

Changelog

  • add goback logic for the ModulesAndOtherTile
  • add logic for wizard step and total step count
  • add test coverage
  • fix the console warning for the flexMap component by adding keys to the children

Review requests

see test plan

Risk assessment

low

@jerader jerader requested a review from a team October 18, 2023 14:53
Comment on lines +176 to +192
} else if (
values.pipettesByMount.right.pipetteName === '' &&
robotType === FLEX_ROBOT_TYPE
) {
goBack(3)
} else {
} else if (
values.pipettesByMount.right.pipetteName === '' &&
robotType === OT2_ROBOT_TYPE
) {
goBack(2)
} else if (
values.pipettesByMount.right.pipetteName !== '' &&
robotType === FLEX_ROBOT_TYPE
) {
goBack(2)
} else {
goBack()
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 know this is very confusing, I am not sure how to clean it up. When the feature flag is removed the options will become clearer I think. So maybe we can keep as is for now and refactor when the ff is removed.

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 the nest isn't deep (only 1 nest) so it would be okay.
When we can remove ff, we can remove this if statement, if (!enableDeckModification || robotType === OT2_ROBOT_TYPE) { right?

@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #13807 (2a71be9) into edge (02190d6) will decrease coverage by 0.10%.
Report is 8 commits behind head on edge.
The diff coverage is 63.63%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #13807      +/-   ##
==========================================
- Coverage   71.31%   71.21%   -0.10%     
==========================================
  Files        2418     2421       +3     
  Lines       67988    68057      +69     
  Branches     8162     8184      +22     
==========================================
- Hits        48483    48470      -13     
- Misses      17556    17634      +78     
- Partials     1949     1953       +4     
Flag Coverage Δ
protocol-designer 45.21% <63.63%> (-0.53%) ⬇️

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

Files Coverage Δ
...onents/modals/CreateFileWizard/PipetteTypeTile.tsx 84.84% <ø> (ø)
...ol-designer/src/components/modules/FlexSlotMap.tsx 90.90% <100.00%> (ø)
...r/src/components/modals/CreateFileWizard/index.tsx 76.47% <92.30%> (+2.00%) ⬆️
...ts/modals/CreateFileWizard/ModulesAndOtherTile.tsx 60.86% <12.50%> (-5.26%) ⬇️

... and 19 files with indirect coverage changes

@koji
Copy link
Contributor

koji commented Oct 18, 2023

probably use overflow-y: auto; would solve this.

Screenshot 2023-10-18 at 5 39 47 PM

@koji
Copy link
Contributor

koji commented Oct 18, 2023

Question about the last case in the test plan
The wizard skips staging area as expected but step jumps up from 4/7 to 7/7.
Is that an expected behavior?

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.

The changes look good to me.
Tested 3 cases and they worked as expected.
I left one question about the step count.

@jerader
Copy link
Collaborator Author

jerader commented Oct 19, 2023

Question about the last case in the test plan The wizard skips staging area as expected but step jumps up from 4/7 to 7/7. Is that an expected behavior?

@koji ya that happens if you don't select a 2nd pipette because you skip the 2nd pipette tip tiles and the staging area tile. So it jumps from step 4 to 7. This might be confusing without the feature flag but hoping 7.1 removes the ff 😅

@jerader jerader merged commit 6708e7a into edge Oct 19, 2023
@jerader jerader deleted the pd_fix-total-step-count branch October 19, 2023 12:49
y3rsh added a commit that referenced this pull request Oct 25, 2023
* edge: (77 commits)
  fix(app): update move gantry text in change pipette flow (#13712)
  fix(app): fix CI after release merge back (#13816)
  chore(hardware): add opentrons hardware package to ot 2 build (#13770)
  feat(protocol-designer): correct step count in create file wizard (#13807)
  feat(protocol-designer): error handling in create file wizard (#13804)
  fix(app): invalidate OT2 calibration queries when calibration flows complete (#13809)
  always jog (#13806)
  chore(app): point updates at dns not s3 (#13798)
  docs(api): updated Flex default flow rates (#13796)
  fix(api): Flag pipette as not ready to aspirate after pushing out air (#13728)
  fix(api, hardware): allow OT3Controller to disable tip motors (#13805)
  more blank trials; longer scale stabilize wait; submerge 2.5mm (#13788)
  fix(app): unload adapters after checking position of labware on adapter on heater shaker module (#13803)
  feat(app): wire up location conflict modal for ODD (#13797)
  feat(app): add Deck configuration page component (#13784)
  fix(robot-server): add default to robot serial (#13802)
  feat(protocol-designer, components): prep work for deck config in PD (#13775)
  feat(app, protocol-designer): plug in waste chute asset (#13800)
  fix(app): do not check mag block in calibration status (#13799)
  feat(protocol-designer): edit additional items staging area support (#13752)
  ...
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.

2 participants