-
Notifications
You must be signed in to change notification settings - Fork 176
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): gather all protocol and calibration data #8182
Conversation
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.
Everything is looking good. These are some gnarly bits of the client that you're untangling and you've tackled it well.
I made a few house keeping suggestions. I'll do another pass over the code, and test it out, once you've got some tests written too.
if (deckCalStatus !== 'OK') { | ||
return { | ||
complete: false, | ||
reason: 'calibrate deck', |
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.
Doesn't have to happen in this PR, but it would be good to capture these reasons in a typed const, that way we can also use them later on as a sort of enum key to the actual tool tip message.
Codecov Report
@@ Coverage Diff @@
## edge #8182 +/- ##
==========================================
- Coverage 74.77% 74.76% -0.02%
==========================================
Files 1668 1670 +2
Lines 44373 44482 +109
Branches 4440 4469 +29
==========================================
+ Hits 33181 33256 +75
- Misses 10438 10471 +33
- Partials 754 755 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
bb0f71b
to
76ad9d3
Compare
f3fb6fa
to
a7da085
Compare
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 merge! 🧇
app/src/organisms/ProtocolSetup/RunSetupCard/RobotCalibrationStep/DeckCalibration.tsx
Outdated
Show resolved
Hide resolved
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.
Great work Sarah!
Left a few comments. I don't want to hold up this PR, but I think we should come back and add some missing test coverage because some of these selectors you wrote are pretty important and we'll rely on them heavily.
Also, I see you moved RobotCalibrationStep
into RunSetupCard
. When I did labware setup I kept LabwareSetup
as it's own directory as a sibling to RunSetupCard
, and I see @jerader did the same for ModuleSetup
.
It doesn't really matter to me which structure we choose, but we should be consistent in where we place each of these steps (since they're siblings), and be consistent in the naming (postfix with Step or not).
app/src/organisms/ProtocolSetup/RunSetupCard/RobotCalibrationStep/DeckCalibration.tsx
Outdated
Show resolved
Hide resolved
app/src/organisms/ProtocolSetup/RunSetupCard/RobotCalibrationStep/DeckCalibration.tsx
Outdated
Show resolved
Hide resolved
app/src/organisms/ProtocolSetup/RunSetupCard/RobotCalibrationStep/index.tsx
Outdated
Show resolved
Hide resolved
@shlokamin I don't have a strong opinion on how these should be organized. I'll drop the postfix Step since I'm last to merge. For file organization I was going off of @b-cooper's suggestion in this comment, which I do think makes sense since all three of these are technically children of |
…calibration step fix #8097
7f42c07
to
ac12926
Compare
fix #8097
Overview
This PR includes all of the selectors needed to gather data on required labware from a json protocol and check that against the labware that is currently on the robot.
Changelog
Review requests
Am I missing any possible reasons that the calibration step is not complete? Is this all of the data we need to consider to display all scenarios?
Risk assessment
Mid - behind feature flag