-
Notifications
You must be signed in to change notification settings - Fork 179
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, components, step-generation): update modules section to accommodate gripper #12955
Conversation
Codecov Report
@@ Coverage Diff @@
## edge #12955 +/- ##
==========================================
- Coverage 72.88% 72.79% -0.09%
==========================================
Files 2342 2350 +8
Lines 64208 64782 +574
Branches 7127 7327 +200
==========================================
+ Hits 46799 47161 +362
- Misses 15741 15902 +161
- Partials 1668 1719 +51
Flags with carried forward coverage won't be shown. Click here to find out more.
|
components/src/slotmap/SlotMap.tsx
Outdated
@@ -13,28 +13,38 @@ export interface SlotMapProps { | |||
collisionSlots?: string[] | |||
/** Optional error styling */ | |||
isError?: boolean | |||
isOt3?: boolean |
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'd prefer this branch on RobotType
, but at the very least can we not refer to this as OT3 (use Flex instead)
@@ -432,7 +432,7 @@ export const DeckSetup = (): JSX.Element => { | |||
}) | |||
|
|||
return ( | |||
<React.Fragment> | |||
<> |
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 this fragment can be deleted completely?
@@ -174,27 +206,40 @@ export function FileSidebar(props: Props): JSX.Element { | |||
command => !LOAD_COMMANDS.includes(command.commandType) | |||
) ?? [] | |||
|
|||
const hasMoveLabware = |
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 also need to check that the move labware is using the gripper here. As written this will trigger for manual moves as well
) | ||
|
||
// TODO(jr, 6/22/23): need to refactor when the gripper toggle is wired up |
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.
isn't the gripper toggle wired up?
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.
oops ya idk why i thought it wasn't wired up 🤦
@@ -5,18 +5,20 @@ import styles from './EditModules.css' | |||
|
|||
interface ConnectedSlotMapProps { | |||
fieldName: string | |||
isOt3: boolean |
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.
same here, can we rename this prop, or make it RobotType
?
<ConnectedSlotMap fieldName={'selectedSlot'} /> | ||
<ConnectedSlotMap | ||
fieldName="selectedSlot" | ||
isOt3={robotType === 'OT-3 Standard'} |
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.
import the FLEX_ROBOT_TYPE
constant to compare against here
@@ -67,18 +75,22 @@ export function EditModulesCard(props: Props): JSX.Element { | |||
].some(pipetteSpecs => pipetteSpecs?.channels !== 1) | |||
|
|||
const warningsEnabled = !moduleRestrictionsDisabled | |||
const isOt3 = robotType === 'OT-3 Standard' |
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.
use FLEX_ROBOT_TYPE
constant
@@ -299,7 +299,9 @@ export const createFile: Selector<ProtocolFile> = createSelector( | |||
} => { | |||
const loadedPipettes = Object.values(pipettes) | |||
const pipetteGEN = loadedPipettes.some( | |||
pipette => getPipetteNameSpecs(pipette.name)?.displayCategory === GEN3 | |||
pipette => |
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 entire method should be replaced with the contents of robotType
key on the protocol file instead of inferring it from pipette types. If you don't want to cram that into this PR, add a TODO
@@ -0,0 +1,7 @@ | |||
export interface SetGripperAction { | |||
type: 'IS_GRIPPER_REQUIRED' |
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.
The naming conventions here are a little wonky. I know it seems redundant, but the action type/function/interface should all reflect the action verb itself. E.g. here I would name them accordingly
interface ToggleIsGripperRequiredAction {
type: 'TOGGLE_IS_GRIPPER_REQUIRED'
}
export const toggleIsGripperRequired ...
@@ -47,7 +47,10 @@ import { getLabwareOnModule } from '../../ui/modules/utils' | |||
import { nestedCombineReducers } from './nestedCombineReducers' | |||
import { PROFILE_CYCLE, PROFILE_STEP } from '../../form-types' | |||
import { Reducer } from 'redux' | |||
import { NormalizedPipetteById } from '@opentrons/step-generation' | |||
import { | |||
NoramlizedAdditionalEquipmentById, |
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.
NoramlizedAdditionalEquipmentById
should be NormalizedAdditionalEquipmentById
) | ||
const hasGripper = gripper.length > 0 | ||
// @ts-expect-error (jr, 6/22/23): OT-3 Standard doesn't exist on schemav6 | ||
const isOt3 = file.robot.model === 'OT-3 Standard' |
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 const should be renamed, and we should be comparing against FLEX_ROBOT_TYPE
instead of the bare string
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.
🏁 🏎️
closes RLIQ-416, RAUT-504, RAUT-506, RAUT-510
Overview
For the Flex protocols, the modules section is updated to allow for adding/removing gripper and the deckmap and edit modules is updated.
Test Plan
Test creating a Flex protocol, modify the modules and gripper and ensure that it works properly. When exporting with a magnetic block, you shouldn't get an "unused module" modal. When exporting with an unused gripper, you sohuld get an "unused gripper" modal.
Test creating an Ot-2 protocol, should work as normal.
Changelog
additionalEquipmentInvariantProperties
key to root state in reduxIS_GRIPPER_REQUIRED
type redux action (toggleIsGripperRequired
), reducer and selector (getAdditionalEquipment
)CreateFileWizard
andEditModulesCard
for adding/removing a grippergripperRow
to the Modules section and rename the section toAdditionalItems
to encompass modules and gripperFileSideBar
container to take in robot type and additional equipment for wiring up logic for unused gripper and unused module modalReview requests
see test plan
Risk assessment
low