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

refactor(app): replace RobotWorkSpace in DeckThumbnail with BaseDeck #13875

Merged
merged 15 commits into from
Nov 7, 2023

Conversation

koji
Copy link
Contributor

@koji koji commented Oct 30, 2023

Overview

replace RobotWorkSpace with BaseDeck

  • ChooseProtocolSlideout
  • ProtocolCard
  • ProtocolDetails
  • ProtocolDetails for ODD

For LabwareInfoOverlay, the component requires runId since LabwareInfo uses runId for useLabwareOffsetForLabware.
useLabwareOffsetForLabware uses runId to get mostRecentAnalysis.

close RAUT-819

Test Plan

open the above components on Desktop app and Touchscreen app

Changelog

  • update DeckThumbnail component and its test
  • add a prop(protocolAnalysis) to the above 4 components

Review requests

Risk assessment

low

replace RobotWorkSpace with BaseDeck

close RAUT-819
@koji koji requested review from brenthagen, jerader and a team October 30, 2023 22:41
@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Merging #13875 (a4b22fc) into edge (c15d30e) will decrease coverage by 0.01%.
Report is 15 commits behind head on edge.
The diff coverage is 65.71%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #13875      +/-   ##
==========================================
- Coverage   70.75%   70.75%   -0.01%     
==========================================
  Files        1629     2506     +877     
  Lines       54062    70592   +16530     
  Branches     3797     8653    +4856     
==========================================
+ Hits        38254    49946   +11692     
- Misses      15143    18531    +3388     
- Partials      665     2115    +1450     
Flag Coverage Δ
app 68.54% <74.19%> (+24.97%) ⬆️
components 63.21% <0.00%> (-0.14%) ⬇️
labware-library 49.17% <ø> (ø)
protocol-designer 45.63% <ø> (+0.07%) ⬆️

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

Files Coverage Δ
app/src/organisms/ChooseProtocolSlideout/index.tsx 75.00% <100.00%> (ø)
app/src/organisms/ProtocolDetails/index.tsx 42.85% <100.00%> (ø)
...rc/pages/OnDeviceDisplay/ProtocolDetails/index.tsx 62.93% <100.00%> (ø)
...pp/src/organisms/ProtocolsLanding/ProtocolCard.tsx 0.00% <0.00%> (ø)
...src/pages/OnDeviceDisplay/ProtocolDetails/Deck.tsx 66.66% <0.00%> (ø)
components/src/hardware-sim/BaseDeck/BaseDeck.tsx 0.00% <0.00%> (ø)
app/src/molecules/DeckThumbnail/index.tsx 77.77% <76.00%> (ø)

... and 875 files with indirect coverage changes

@koji koji marked this pull request as ready for review October 31, 2023 18:28
@koji koji requested a review from a team as a code owner October 31, 2023 18:28
@koji koji requested a review from brenthagen October 31, 2023 18:28
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.

looks good so far, but we need to add liquid info also. we don't have an example BaseDeck implementation of liquids yet - will be similar to RAUT-821

app/src/molecules/DeckThumbnail/index.tsx Outdated Show resolved Hide resolved
app/src/molecules/DeckThumbnail/index.tsx Outdated Show resolved Hide resolved
@koji koji requested review from brenthagen and a team November 3, 2023 15:58
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.

with these changes, we should end up with a fairly simple <BaseDeck {...someProps} /> instance (with no children) in the render of DeckThumbnail

app/src/molecules/DeckThumbnail/index.tsx Show resolved Hide resolved
module.moduleDef.model === THERMOCYCLER_MODULE_V1
? { lidMotorState: 'open' }
: {},
nestedLabwareDef: topLabwareDefinition,
Copy link
Contributor

Choose a reason for hiding this comment

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

here we want to add the well fill to the nested labware - add a prop nestedLabwareWellFill to moduleLocations in BaseDeck

labwareLocation: { slotName },
definition: topLabwareDefinition,
topLabwareId,
topLabwareDisplayName,
Copy link
Contributor

Choose a reason for hiding this comment

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

simliar, we want to add the well fill to the labware - add a prop wellFill to labwareLocations in BaseDeck

moduleInSlot.result.moduleId in initialLoadedLabwareByModuleId
? initialLoadedLabwareByModuleId[moduleInSlot.result.moduleId]
: null
{map(
Copy link
Contributor

Choose a reason for hiding this comment

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

all of the children content here inside BaseDeck should not not be needed if we are processing moduleLocations and labwareLocations correctly above - we should be able to use

          const wellFill = getWellFillFromLabwareId(
            topLabwareId ?? '',
            liquids,
            labwareByLiquidId
          )
          const labwareHasLiquid = !isEmpty(wellFill)

in those two locations above with the added well fill props

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brenthagen
updated code based on your comment and one question about labwareHasLiquid.
Do we need to add interactions that only exists in DeckView of SetupLiquidsMap?

Screenshot 2023-11-06 at 1 47 25 PM

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 no, we don't need to add liquid hover or click interactions to DeckThumbnail. so we won't need labwareHasLiquid in DeckThumbnail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, thank you!

@brenthagen
Copy link
Contributor

brenthagen commented Nov 3, 2023

also, it would be a good idea to change the title of this PR to indicate it is only doing a replacement in DeckThumbnail

@koji koji changed the title refactor(app): replace RobotWorkSpace with BaseDeck refactor(app): replace RobotWorkSpace in DeckThumbnail with BaseDeck Nov 6, 2023
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.

looks good, comments on slot labels and hover could be addressed in a follow on PR if wanted

app/src/molecules/DeckThumbnail/index.tsx Outdated Show resolved Hide resolved
components/src/hardware-sim/BaseDeck/BaseDeck.tsx Outdated Show resolved Hide resolved
@koji koji merged commit d7c25b2 into edge Nov 7, 2023
42 of 44 checks passed
@koji koji deleted the app_refactor-replace-robotworkspace-with-basedeck branch November 17, 2023 21:28
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