feat(app): make protocol runs from history clickable #10537
Conversation
jerader
requested review from
brenthagen,
smb2268,
b-cooper and
koji
and removed request for
a team
Codecov Report
@@ Coverage Diff @@
## edge #10537 +/- ##
==========================================
+ Coverage 73.81% 73.85% +0.04%
==========================================
Files 2139 2141 +2
Lines 57526 57889 +363
Branches 5807 5945 +138
==========================================
+ Hits 42460 42756 +296
- Misses 13846 13871 +25
- Partials 1220 1262 +42
Flags with carried forward coverage won't be shown. Click here to find out more.
|
jerader
force-pushed
the
app_protocol-runs-add-buttons
branch
from
982b7f5
to
0abd5b4
Compare
| interface HistoricalProtocolRunProps { | ||
| run: RunData | ||
| protocolKey?: string |
There was a problem hiding this comment.
Optional props should go last probably.
There was a problem hiding this comment.
ooh i've never really thought about that/knew this was the convention. But it makes sense to make it more readable.
| const protocolKeys = storedProtocols.map(({ protocolKey }) => protocolKey) | ||
| const protocolKeyInStoredKeys = Object.values(protocolKeys).find( | ||
| key => key === protocolKey | ||
| ) | ||
|
|
There was a problem hiding this comment.
Object.values returns an array. protocolKeys here is an array so you don't need to use that. I think this can also be simplified to the following:
const protocolKeyInStoredKeys = storedProtocols
.map(({ protocolKey }) => protocolKey)
.find(key => key === protocolKey)
| @@ -43,6 +58,10 @@ export function HistoricalProtocolRun( | |||
| duration = formatInterval(run.createdAt, new Date().toString()) | |||
| } | |||
| } | |||
| const protocolKeyInStoredKeys = storedProtocols | |||
| .map(({ protocolKey }) => protocolKey) | |||
There was a problem hiding this comment.
oooo, there's already a variable called protocolKey in this component. we should be able to avoid this confusion by just simplifying this to one .find call
e.g. storedProtocols.find(({protocolKey: key}) => protocolKey === key)
this allows us to destructure out the value in the protocolKey of each storedProtocol, but immediately alias it to key so we don't have to worry about the name collision
There was a problem hiding this comment.
That makes sense! I'll make that change. Thanks!
| > | ||
| {protocolName} | ||
| </StyledText> | ||
| {protocolKeyInStoredKeys != null ? ( |
There was a problem hiding this comment.
It looks like this ternary statement could be moved within the onClick callback of the protocolName as that's the only thing that actually switches on whether or not the protocolKey was found in a locally stored protocol.
It also looks like there is a bit of unintentional functionality here. This currently only allows a user to link to the run record, if the associated protocol exists locally on the app. There is no reason for this restriction, a user should be able to view the run detail page for any run record, regardless of whether or not the protocol with the corresponding key is present on the file system.
There was a problem hiding this comment.
Oh, i misunderstood the AC. I thought we didn't want to be able to access the run record if the protocol was deleted. Will fix that now. Thanks.
|
go into Protocol Cards and delete a protocol. When you return to the Protocol Runs, the run and protocol name for that deleted protocol should not be clickable.
|
closes #10502
Overview
This PR makes each run and protocol in the run history clickable. When clicking on run, it links to the run details page. When clicking on protocol Name, it links to the protocol details page.
Changelog
HistoricalProtocolRunto take in a protocol key prop and also gets stored protocolsRecentProtocolRunsReview requests
Risk assessment
low