feat(odd): Protocol dashboard hifi designs #12453
Conversation
Screen.Recording.2023-04-10.at.9.51.07.AM.mov |
Upload.from.GitHub.for.iOS.MOV |
Codecov Report
@@ Coverage Diff @@
## edge #12453 +/- ##
==========================================
- Coverage 73.81% 73.78% -0.04%
==========================================
Files 2246 2246
Lines 61740 61772 +32
Branches 6430 6439 +9
==========================================
+ Hits 45574 45577 +3
- Misses 14653 14679 +26
- Partials 1513 1516 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ewagoner
changed the title
| full: { | ||
| fontSize: TYPOGRAPHY.fontSize32, | ||
| height: '100%', | ||
| lineHeight: TYPOGRAPHY.lineHeight42, | ||
| width: '100%', | ||
| }, | ||
| half: { | ||
| fontSize: TYPOGRAPHY.fontSize28, | ||
| height: '10.75rem', | ||
| lineHeight: TYPOGRAPHY.lineHeight36, | ||
| width: '29.25rem', | ||
| }, | ||
| regular: { | ||
| fontSize: TYPOGRAPHY.fontSize28, | ||
| height: '10.75rem', | ||
| lineHeight: TYPOGRAPHY.lineHeight36, | ||
| width: '28.375rem', | ||
| }, |
There was a problem hiding this comment.
maybe we can define this as a const like cardSizeOptions and put it right after import statements?
app/src/pages/OnDeviceDisplay/ProtocolDashboard/PinnedProtocol.tsx
Outdated
Show resolved
Hide resolved
app/src/pages/OnDeviceDisplay/ProtocolDashboard/PinnedProtocol.tsx
Outdated
Show resolved
Hide resolved
| const cardSize = | ||
| pinnedProtocols.length === 1 | ||
| ? 'full' | ||
| : pinnedProtocols.length === 2 | ||
| ? 'half' | ||
| : 'regular' |
There was a problem hiding this comment.
I think this is totally fine since I like this personally.
But, maybe switch could be a better option?
what do you think @shlokamin @jerader?
There was a problem hiding this comment.
I turned it into a boring function without the nesting.
| const runs = useAllRunsQuery() | ||
| const swipe = useSwipe() | ||
|
|
||
| const slider = (swipe.ref.current as unknown) as HTMLDivElement |
There was a problem hiding this comment.
@koji can we avoid this double as casting if we change the type of interactiveRef in useSwipe to MutableRefObject< HTMLDivElement | null> ?
| let isDown = false | ||
| let startX = 0 | ||
| let scrollLeft = 0 |
There was a problem hiding this comment.
hmm is there a reason we're using global variables rather than refs here? makes my spidey senses tingle
There was a problem hiding this comment.
Only that I was torn between making this fully generic and reusable carousel component and one that is only used by the pinned protocols. I wrote two different sets of functions, since the ODD swiping and mousing works differently. With a mouse, I initialize the dragging positions at 0 and move left or right based on mouse movement. With the ODD swipe, I use the ref and just move it 800px each swipe (the most "natural" feeling amount of movement based on my tests but still totally arbitrary.
There wasn't any harm in just initializing mouse movement with zeros for now, but if this carousel ends up getting used elsewhere and for things other than pinned protocols, that might have to be parameterized.
| const cardSize = | ||
| pinnedProtocols.length === 1 | ||
| ? 'full' | ||
| : pinnedProtocols.length === 2 | ||
| ? 'half' | ||
| : 'regular' |
There was a problem hiding this comment.
nit: nested ternaries make me sad. can we extract this into a function and make this switch/if statement?
There was a problem hiding this comment.
I've simplified this brain teaser :)
|
also looks like you need to rebase on top of edge to resolve these merge conflicts |
Oh, those are brand new! I'll fix 'em. |
ewagoner
force-pushed
the
RCORE-673-odd-protocol-dashboard
branch
from
ee72500
to
c498082
Compare
|
looks we need to wrap pinned protocol name's name. my suggestion for now is below. |
I am truncating the name. For a single pinned protocol it will max out at two lines. For two or more pinned protocols, it can max out at three lines. The truncation rules were in Figma but they didn't quite align with the max lines rule so I adjusted them in my code. You can see them in action in my videos attached to this PR. I've got one example with a crazy long name that demonstrates the truncation. Your example is a word break issue, which would apply to any fixed width div. |
I've pushed a fix for the wrap issue |
app/src/pages/OnDeviceDisplay/ProtocolDashboard/__tests__/ProtocolRow.test.tsx
Show resolved
Hide resolved
app/src/pages/OnDeviceDisplay/ProtocolDashboard/PinnedProtocol.tsx
Outdated
Show resolved
Hide resolved
|
Probably need to solve conflicts since Jethary merged her PR which is related to |
ewagoner
force-pushed
the
RCORE-673-odd-protocol-dashboard
branch
from
a253e06
to
ef4cecc
Compare
Overview
This PR implements the HiFi designs for the protocol dashboard, as specified here: https://www.figma.com/file/AoTLAYuWawlaWItB1umOjr/Opentrons-Flex-Touchscreen-Designs?node-id=5155-84968&t=WIbrmZjlwG5NxXMe-0
TODO: The protocol card component needs to display analysis info, and I don't believe that's available yet. It's also not quite a ListItem, but when analysis is ready to display it's possible this could use the generic ListItem.
Closes RCORE-673
Test Plan
This PR is almost purely UI changes. Core logic was already in place with the LoFi implementation and contains unit tests. I will go through the coverage reports and if there are uncovered bits I will add additional unit tests to this PR.
Changelog
Review requests
Please give it a spin on the DevKit and report anything amiss.
Risk assessment
This only implements UI changes over the LoFi implementation, so user risk is minimal.