-
Notifications
You must be signed in to change notification settings - Fork 180
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): ODD protocol detail and protocol setup status for deck configuration #13855
Conversation
…configuration ODD: Render the correct modules and deck status taking into account location conflicts, missing hardware, and missing calibrations. Render this status in both ProtocolDetails (chipText) and ProtocolSetup (Modules & deck) pages. Extend hooks to look for conflicting or missing hardware
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## edge #13855 +/- ##
==========================================
- Coverage 70.86% 70.72% -0.14%
==========================================
Files 2501 1624 -877
Lines 70338 53755 -16583
Branches 8600 3775 -4825
==========================================
- Hits 49844 38020 -11824
+ Misses 18398 15081 -3317
+ Partials 2096 654 -1442
Flags with carried forward coverage won't be shown. Click here to find out more. |
app/src/pages/OnDeviceDisplay/ProtocolSetup/__tests__/ProtocolSetup.test.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.
looking good to far! The main thing I noticed in your i18n strings is you're inconsistent when you use {{count}}
vs {{num}}
. Since count
is associated with the plural usage, it would be good to only use {{count}}
when using plural
.
app/src/pages/OnDeviceDisplay/ProtocolSetup/__tests__/ProtocolSetup.test.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.
left a few comments - main one is to make sure we're only making changes to protocol setup status here
app/src/organisms/OnDeviceDisplay/RobotDashboard/hooks/useHardwareStatusText.ts
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.
code makes sense to me, checked a few different states but not all - let's be sure to comment out the isReadyToRun
condition before merging
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.
LGTM !
closes RAUT-667
Overview
The ODD ProtocolSetup page should render the correct module and deck status taking into account module calibration and connection, and now deck configuration. We need to check for the current deck configuration state to ensure the protocol doesn't require hardware where a fixture currently exists, and alert the user of a location conflict if necessary. The status will also render modules and/or fixtures are missing. This hardware status will also be rendered as the chipText on the ProtocolDetails page.
Test Plan
Testing all cases below on a physical Flex
Protocol requires neither modules nor fixtures
Protocol requires only modules
Protocol requires only fixtures
Protocol requires both modules and fixtures
Changelog
Review requests
@brenthagen as discussed in person
Risk assessment
medium. Main risk lies in the enabling/disabling of the protocol Play button based on new deck configuration considerations