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(app): wire up location conflict modal for ODD #13797

Merged
merged 5 commits into from
Oct 17, 2023

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Oct 16, 2023

closes RAUT-673

Overview

This PR wires up the location conflict modal for the ODD and changes the modal to match ODD designs. Additionally, the ProtocolSetupModules folder and component is renamed to ProtooclSetupModulesAndDeck

Test Plan

Test on an ODD, go to protocol setup and with a location conflict for fixture and module, click the row to reveal the modal. Make sure the modal works and looks as expected.

Changelog

  • change ProtocolSetupModules component to ProtocolSetupModulesAndDeck
  • add code to the location conflict modal to include ODD styling
  • wire up the modal to the 2 list view components for the ODD
  • add test coverage

Review requests

see test plan

Risk assessment

low

@jerader jerader marked this pull request as ready for review October 16, 2023 20:45
@jerader jerader requested a review from a team as a code owner October 16, 2023 20:45
@jerader jerader requested review from TamarZanzouri, koji, brenthagen and a team and removed request for a team and TamarZanzouri October 16, 2023 20:45
@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #13797 (1cfa446) into edge (014c0ca) will increase coverage by 0.01%.
Report is 3 commits behind head on edge.
The diff coverage is 72.22%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #13797      +/-   ##
==========================================
+ Coverage   70.74%   70.76%   +0.01%     
==========================================
  Files        2468     2469       +1     
  Lines       69459    69565     +106     
  Branches     8373     8405      +32     
==========================================
+ Hits        49139    49226      +87     
- Misses      18348    18361      +13     
- Partials     1972     1978       +6     
Flag Coverage Δ
app 68.20% <72.22%> (+0.08%) ⬆️
protocol-designer 45.82% <ø> (ø)

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

Files Coverage Δ
app/src/organisms/ProtocolSetupLabware/index.tsx 50.00% <ø> (ø)
...ocolSetupModulesAndDeck/SetupInstructionsModal.tsx 100.00% <ø> (ø)
...src/organisms/ProtocolSetupModulesAndDeck/utils.ts 42.10% <ø> (ø)
.../src/pages/OnDeviceDisplay/ProtocolSetup/index.tsx 76.59% <ø> (ø)
...olRun/SetupModuleAndDeck/LocationConflictModal.tsx 85.00% <85.71%> (-2.50%) ⬇️
...nisms/ProtocolSetupModulesAndDeck/FixtureTable.tsx 81.81% <66.66%> (ø)
...rc/organisms/ProtocolSetupModulesAndDeck/index.tsx 65.90% <60.00%> (ø)

... and 8 files with indirect coverage changes

Comment on lines 125 to 142
<Flex
padding={SPACING.spacing24}
backgroundColor={COLORS.light1}
flexDirection={DIRECTION_ROW}
alignItems={ALIGN_CENTER}
justifyContent={JUSTIFY_SPACE_BETWEEN}
borderRadius={BORDERS.borderRadiusSize3}
>
<StyledText as="p" fontWeight={TYPOGRAPHY.fontWeightSemiBold}>
{t('protocol_specifies')}
</StyledText>

<StyledText as="p">
{requiredFixture && getFixtureDisplayName(requiredFixture)}
{requiredModule && getModuleDisplayName(requiredModule)}
</StyledText>
</Flex>
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

<ListItem type="noActive">
  <Flex flexDirection={DIRECTION_ROW} justifyContent={JUSTIFY_SPACE_BETWEEN}>
    <StyledText as="p" fontWeight={TYPOGRAPHY.fontWeightSemiBold}>
                    {t('protocol_specifies')}
                  </StyledText>
                  <StyledText as="p">
                    {requiredFixture && getFixtureDisplayName(requiredFixture)}
                    {requiredModule && getModuleDisplayName(requiredModule)}
                  </StyledText>
  <Flex>
</ListItem>

I should update ListItem since currently it supports only one text case and need to pass noActive as the default type.
ListItem was created when I worked on E-stop modal.

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.

lgtm
I just left a couple of comments.

@koji
Copy link
Contributor

koji commented Oct 17, 2023

nit
maybe using gripGap makes the code easy to read.

<Flex flexDirection={} gridGap={SPACING.spacing32}>
  <StyledText>{description}</StyledText>
  <Flex>
    <StyledText>Slot {}</StyledText>
    <ListItem />
    <ListItem />
  </Flex>
  <Flex>
    <SmallButton />
    <SmallButton />
  </Flex>
</Flex>

Screenshot 2023-10-17 at 3 23 17 PM

@koji koji closed this Oct 17, 2023
@koji koji reopened this Oct 17, 2023
@koji
Copy link
Contributor

koji commented Oct 17, 2023

sorry mistakenly clicked close instead of comment.

Copy link
Contributor

@brenthagen brenthagen left a comment

Choose a reason for hiding this comment

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

a couple minor comments, LGTM !

@jerader jerader force-pushed the app_protocol-setup-mod-and-deck-off branch from f2016b8 to 1cfa446 Compare October 17, 2023 19:50
@jerader jerader merged commit abdd067 into edge Oct 17, 2023
@jerader jerader deleted the app_protocol-setup-mod-and-deck-off branch October 17, 2023 20:26
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.

3 participants