feat(app): add bare bones hardware section to protocol details #12099
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## edge #12099 +/- ##
==========================================
+ Coverage 73.97% 74.04% +0.06%
==========================================
Files 2198 2201 +3
Lines 60912 61147 +235
Branches 6477 6579 +102
==========================================
+ Hits 45058 45274 +216
- Misses 14317 14325 +8
- Partials 1537 1548 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
|
koji
reviewed
Feb 3, 2023
app/src/App/OnDeviceDisplayApp.tsx
Outdated
| @@ -20,7 +20,7 @@ import { TempODDMenu } from '../pages/OnDeviceDisplay/TempODDMenu' | |||
| import { RobotDashboard } from '../pages/OnDeviceDisplay/RobotDashboard' | |||
| import { RobotSettingsDashboard } from '../pages/OnDeviceDisplay/RobotSettingsDashboard' | |||
| import { ProtocolDashboard } from '../pages/OnDeviceDisplay/ProtocolDashboard' | |||
| import { ProtocolDetails } from '../pages/OnDeviceDisplay/ProtocolDetails' | |||
| import { ProtocolDetails } from '../pages/OnDeviceDisplay/ProtocolDetails/ProtocolDetails' | |||
There was a problem hiding this comment.
maybe changing the file name from ProtocolDetails.tsx to index.tsx?
koji
reviewed
Feb 3, 2023
Comment on lines
+8
to
+11
| import { | ||
| getModuleDisplayName, | ||
| getPipetteNameSpecs, | ||
| } from '@opentrons/shared-data' |
There was a problem hiding this comment.
maybe these lines after @opentrons/components?
koji
reviewed
Feb 3, 2023
| text-align: left; | ||
| ` | ||
| const TableHeader = styled('th')` | ||
| text-transform: ${TYPOGRAPHY.textTransformUppercase}; |
There was a problem hiding this comment.
From the design capitalize, but the design is mid-fi.
we don't need to change this right now.
koji
reviewed
Feb 3, 2023
| ` | ||
| const TableHeader = styled('th')` | ||
| text-transform: ${TYPOGRAPHY.textTransformUppercase}; | ||
| color: ${COLORS.darkBlackEnabled}; |
There was a problem hiding this comment.
The default color is darkBlackEnabled.
We can remove this.
https://github.com/Opentrons/opentrons/blob/edge/app/src/atoms/GlobalStyle/index.ts#L19
koji
reviewed
Feb 3, 2023
|
|
||
| const displayName = | ||
| protocolRecord?.data.metadata.protocolName ?? | ||
| protocolRecord?.data.files[0].name | ||
|
|
||
| return ( | ||
| <Flex flexDirection="column" alignItems={JUSTIFY_FLEX_START}> | ||
| <Flex flexDirection="column" padding={SPACING.spacing6}> |
There was a problem hiding this comment.
Suggested change
| <Flex flexDirection="column" padding={SPACING.spacing6}> | |
| <Flex flexDirection={DIRECTION_COLUMN} padding={SPACING.spacing6}> |
koji
reviewed
Feb 3, 2023
| hardwareType: 'pipette' | ||
| pipetteName: PipetteName | ||
| mount: 'left' | 'right' | ||
| connected: true | false |
There was a problem hiding this comment.
Is there any specific reason to use true | false instead of boolean?
There was a problem hiding this comment.
lol no, i was thinking too literally 😛
koji
approved these changes
Feb 3, 2023
shlokamin
commented
Feb 6, 2023
| @@ -25,7 +29,7 @@ export type Pipettes = FetchPipettesResponseBody | |||
| export type FetchPipettesResponsePipette = | |||
| | { | |||
| id: string | |||
| name: string | |||
| name: PipetteName | |||
y3rsh
added a commit
that referenced
this pull request
Feb 7, 2023
* edge: (116 commits) feat(system-server): add sqlite database and barebones HTTP server (#12085) feat(ot3): add enableOT3FirmwareUpdates feature flag to gate firmware update functionality. (#12102) feat(app): add bare bones hardware section to protocol details (#12099) feat(app): Support failed calibrations in the calibration wizard (#12092) refactor(docs): clean up Versioning page (#12084) refactor(robot-server): Make run and protocol limits configurable at launch (#12094) feat(app): add robotServerVersion to display the current robot software version (#12096) fix(hardware): do not track tip motor positions (#12093) feat(engine): allow calibrateGripper command to save calibration data (#12046) feat(app, api-client, react-api-client): delete POC TLC calibration data from overflow menu (#12075) fix(api): actually update OT3 instrument calibration offset in cache instrument (#12089) fix(robot-server): correct the data returned from instruments endpoint (#12067) feat(api): add thermocycler plate lift to hardware controller (#12068) fix(app): reference moduleId from result not params (#12077) feat(app): create ODD protocol setup page (#12071) refactor(api): Deprecate presses and increment args when using PAPI pick_up_tip (#12079) fix(ot3): handle multiple responses for a tip action request (#12083) refactor(api): touch tip implementation for PAPIv2 engine core (#12053) refactor(app): Remove ssid parameter from OnDeviceRouteParams (#11930) fix(api): fix broken test in the api hardware controller (#12080) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR adds a bare bones hardware section to the protocol details page. Will update implementation when @brenthagen finishes breaking out logic from
useProtocolDetailsForRunwhich is why theres no test coverage for the hook i made. I'll add some unit tests for the components though.closes RCORE-582
Test Plan
I added a few protocols to my dev robot, opened them in the protocol details page on ODD app, and verified that the page rendered the correct info.
Review requests
Add some protocols to your dev robot and look at the protocol details hardware tab. make sure the right info shows up:
Risk assessment
Low