-
Notifications
You must be signed in to change notification settings - Fork 185
feat(app): Remove labware logic from deck conflict resolution checker #18676
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## edge #18676 +/- ##
==========================================
- Coverage 25.72% 25.65% -0.08%
==========================================
Files 3280 3284 +4
Lines 283602 285655 +2053
Branches 28598 29729 +1131
==========================================
+ Hits 72964 73291 +327
- Misses 210611 212337 +1726
Partials 27 27
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like good simplification overall, though there's a couple things around checking for exactly-one conflicts that makes me raise my eyebrow a bit
cutoutFixtureId === THERMOCYCLER_V2_FRONT_FIXTURE && | ||
missingLabwareDisplayName != null | ||
compatibleCutoutFixtureIds.length === 1 && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels a bit fragile, does it need to be exactly 1? or does it need to be 1 or more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like could it be compatibleCutoutFixtureIds.some(fixtureId => SINGLE_SLOT_FIXTURES.includes(fixtureId))
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that would also work, but I do think it will always be exactly 1 and this is closer to the logic that was being used to create that missingLabwareDisplayName
variable. I can change it though and verify it still works as expected!
app/src/organisms/Desktop/Devices/ProtocolRun/SetupModuleAndDeck/SetupFixtureList.tsx
Outdated
Show resolved
Hide resolved
app/src/organisms/Desktop/Devices/ProtocolRun/SetupModuleAndDeck/utils.ts
Show resolved
Hide resolved
app/src/organisms/ODD/ProtocolSetup/ProtocolSetupModulesAndDeck/FixtureTable.tsx
Outdated
Show resolved
Hide resolved
@@ -614,6 +615,12 @@ export function getFixtureDisplayName( | |||
FLEX_STACKER_MODULE_V1 | |||
)} in USB-${usbPortNumber} and magnetic block` | |||
: `${getModuleDisplayName(FLEX_STACKER_MODULE_V1)} and magnetic block` | |||
case SINGLE_CENTER_SLOT_FIXTURE: | |||
return 'Center slot' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do these need translations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should talk to Anthony about this before release. We'd made a call when doing the first iteration of translation work that deck config should not be translated, but maybe we want to revisit that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
fix EXEC-1470
Overview
This PR refactors the deck conflict resolution logic in Protocol Setup Deck Hardware section to no longer find the labware that will eventually need to be placed in an empty slot. Instead, we'll display the specific slot (left, right or center) that needs to be present if there is a conflict.
Test Plan and Hands on Testing
Look at protocol setup for a variety of configurations when your deck config does and doesn't match what is needed for a protocol. Especially:
Changelog
getStackedItemsOnStartingDeck
that isn't super relevant to these changes but I noticed the issue when testing themNeeded to address in a follow-up:
Adding images for these new slot fixtures once we have them from design
Review requests
Look over the changes and test this out if you can! I've done a good amount of testing but deck config has a lot of possible variations so the more testing the better.
Risk assessment
Medium