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(protocol-designer): ot-2 slot map is now a normal size #14518

Merged
merged 4 commits into from
Feb 21, 2024

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Feb 16, 2024

closes RAUT-1005

Overview

See the ticket. no idea when this error was introduced - apparently after i did the migration to react-hook-form?? Weird. But anyway, I fixed it here and also cleaned up the SlotMap components, getting rid of the no longer needed ConnectSlotMap

Bonus: also there was a small bug with the well selection field where it was defaulting the value to 0 which is different from PD in prod. So i fixed that up really quick too.

Test Plan

Create an Ot-2 protocol and edit the modules. See that the slot map is no longer way too big.

Changelog

  • clean up the SlotMap component and associated components
  • fix test
  • fix bug with well selection field where it was defaulting the value to 0 if the length of well array was 0.

Review requests

see test plan

Risk assessment

low

@jerader jerader requested a review from a team as a code owner February 16, 2024 19:11
@jerader jerader requested review from a team and TamarZanzouri and removed request for a team February 16, 2024 19:11
@jerader jerader changed the title refactor(protocol-designer): ot-2 slot map is now a now a normal size refactor(protocol-designer): ot-2 slot map is now a normal size Feb 16, 2024
@jerader jerader removed the request for review from TamarZanzouri February 16, 2024 19:11
Copy link

codecov bot commented Feb 16, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (cb0d4e6) 67.81% compared to head (a4ff7c0) 67.81%.
Report is 20 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #14518      +/-   ##
==========================================
- Coverage   67.81%   67.81%   -0.01%     
==========================================
  Files        2518     2517       -1     
  Lines       72108    72154      +46     
  Branches     9274     9274              
==========================================
+ Hits        48898    48928      +30     
- Misses      20995    21008      +13     
- Partials     2215     2218       +3     
Flag Coverage Δ
app 64.64% <ø> (-0.01%) ⬇️
labware-library 41.11% <ø> (ø)

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

Files Coverage Δ
...r/src/components/modals/EditModulesModal/index.tsx 70.96% <100.00%> (+0.31%) ⬆️
...ocol-designer/src/components/modules/ModuleRow.tsx 0.00% <ø> (ø)
...m/fields/WellSelectionField/WellSelectionField.tsx 2.85% <0.00%> (ø)

... and 23 files with indirect coverage changes

Comment on lines +43 to +46
const primaryWellCount =
Array.isArray(selectedWells) && selectedWells.length > 0
? selectedWells.length.toString()
: undefined
Copy link
Collaborator Author

@jerader jerader Feb 16, 2024

Choose a reason for hiding this comment

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

unrelated to the bug but this fixed another small bug i found that was introduced by my PR that refactored class components into functional components 😬

Copy link
Contributor

@mjhuff mjhuff 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! IMO, I wouldn't be opposed to naming SlotMap to something like OT2SlotMap or leaving a docstring explaining that this is an OT-2 specific component, since we have an analogous FlexSlotMap.

@jerader
Copy link
Collaborator Author

jerader commented Feb 21, 2024

Looks good! IMO, I wouldn't be opposed to naming SlotMap to something like OT2SlotMap or leaving a docstring explaining that this is an OT-2 specific component, since we have an analogous FlexSlotMap.

@mjhuff good call! thank you! SlotMap used to be used for both robot types so I forgot that I made a FlexSlotMap later. I'll rename it to OT2SlotMap

@jerader jerader merged commit 9db4b55 into edge Feb 21, 2024
31 of 32 checks passed
@jerader jerader deleted the pd_fix-ot2-deck-map branch February 21, 2024 16:04
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