Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(app): DQA fixes for protocol details pages #12922

Merged
merged 7 commits into from
Jun 21, 2023
Merged

fix(app): DQA fixes for protocol details pages #12922

merged 7 commits into from
Jun 21, 2023

Conversation

ewagoner
Copy link
Contributor

@ewagoner ewagoner commented Jun 15, 2023 •
edited
Loading

Overview

This PR addresses the DQA feedback provided on RAUT-451 for the Protocol Details pages.

Closes RAUT-451

Test Plan

Visually verified changes, made storybook changes when needed, and changed unit tests to match new work

Changelog

  • Added iconName to module definitions
  • Tweaked LocationIcon to make border optional
  • Tweaked table spacing and colors for labware section
  • Fixed spacing and borders for protocol details header
  • Fixed spacing and colors for summary section
  • Tweaked spacing and iconography tweaks for hardware section

Review requests

Please compare the protocol details pages to the requested revisions on RAUT-451 (including my comments on that ticket)

Risk assessment

Low

Screen.Recording.2023-06-15.at.2.27.25.PM.mov

@ewagoner ewagoner requested review from koji, shlokamin and a team June 15, 2023 18:43
@ewagoner ewagoner requested a review from a team as a code owner June 15, 2023 18:43
@ewagoner
Copy link
Contributor Author

@shlokamin I've tagged you specifically as a reviewer because I've made a change to the module schema to make it easier to display the correct icon as needed and that seems like something you should bless or shame.

@codecov
Copy link

codecov bot commented Jun 15, 2023 •
edited
Loading

Codecov Report

Merging #12922 (f84f201) into edge (dcb6ace) will increase coverage by 0.01%.
The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #12922      +/-   ##
==========================================
+ Coverage   73.30%   73.31%   +0.01%     
==========================================
  Files        2324     2332       +8     
  Lines       63708    64119     +411     
  Branches     6984     7172     +188     
==========================================
+ Hits        46699    47011     +312     
- Misses      15354    15417      +63     
- Partials     1655     1691      +36
Flag Coverage Δ
app 71.61% <81.25%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../pages/OnDeviceDisplay/ProtocolDetails/Labware.tsx 88.23% <ø> (ø)
...rc/pages/OnDeviceDisplay/ProtocolDetails/index.tsx 66.66% <78.57%> (+2.74%) ⬆️
...pages/OnDeviceDisplay/ProtocolDetails/Hardware.tsx 65.21% <100.00%> (-6.22%) ⬇️

... and 27 files with indirect coverage changes

@ewagoner
Copy link
Contributor Author

I've made a change to the module schema to make it easier to display the correct icon

The desktop app uses a different component and has a hardcoded map of modules to icons. Seems like this is something the module should know about itself (especially since the icons are now used in several places across app platforms).

From components/src/icons/ModuleIcon.tsx:

const MODULE_ICON_NAME_BY_TYPE: { [type in ModuleType]: IconName } = {
  [MAGNETIC_BLOCK_TYPE]: 'ot-magnet-v2',
  [HEATERSHAKER_MODULE_TYPE]: 'ot-heater-shaker',
  [TEMPERATURE_MODULE_TYPE]: 'ot-temperature-v2',
  [MAGNETIC_MODULE_TYPE]: 'ot-magnet-v2',
  [THERMOCYCLER_MODULE_TYPE]: 'ot-thermocycler',
}

@ewagoner ewagoner requested a review from koji June 15, 2023 22:24
@ewagoner ewagoner force-pushed the RAUT-451-protocol-detail-dqa-2 branch 2 times, most recently from 06cc817 to fadbd07 Compare June 16, 2023 00:29
@ewagoner ewagoner marked this pull request as draft June 20, 2023 15:45
@ewagoner ewagoner force-pushed the RAUT-451-protocol-detail-dqa-2 branch from fadbd07 to f6ed177 Compare June 21, 2023 14:25
@ewagoner ewagoner force-pushed the RAUT-451-protocol-detail-dqa-2 branch from f6ed177 to 2aa99a9 Compare June 21, 2023 15:35
@ewagoner
Copy link
Contributor Author

you should bless or shame.

Having been sufficiently shamed, this bit has been taken out and replaced with the mapping we already had in the ModuleIcon component.

@ewagoner ewagoner marked this pull request as ready for review June 21, 2023 15:39
@ewagoner ewagoner requested a review from a team June 21, 2023 15:39
koji
koji reviewed Jun 21, 2023
View reviewed changes
app/src/pages/OnDeviceDisplay/ProtocolDetails/index.tsx
width="42.125rem"
>
<Btn
paddingLeft="0rem"
paddingRight={SPACING.spacing20}
paddingRight={SPACING.spacing24}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably spacing16?

Screenshot 2023-06-21 at 12 45 54 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 16 gap is in the Flex gridGap a couple lines up: gridGap={SPACING.spacing16}. This padding of 24 is required to give the entire button a touch target of 48x48 with the chevron on the left half.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh okay. that makes sense.
thank you for your explanation.

koji
koji reviewed Jun 21, 2023
View reviewed changes
app/src/pages/OnDeviceDisplay/ProtocolDetails/index.tsx Outdated
const [truncate, setTruncate] = React.useState<boolean>(true)
const toggleTruncate = (): void => setTruncate(value => !value)

let displayedTitle = title
if (title.length > 92 && truncate) {
displayedTitle = truncateString(title, 92, 69)
displayedTitle = truncateString(title, 69)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this usage, the protocol name is displayed like below
long name...

when passing maxLen and breakPoint
long na... me

so probably we would need to do like this from the Figma.

displayedTitle = truncateString(title, 75, 55)

Screenshot 2023-06-21 at 1 04 04 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works. I swear the truncation had changed to being at the end of the string when I made that edit, but it's not that way now. I blame all the absinthe I drank in my youth.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wait, the size is 16%.
I tested truncateString(title, 86, 63) from the font-size 75/(38/44) and the name was 4-line.
truncateString(title, 80, 60) would be better than (75,55)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do that to move things forward. It depends on character size too, so a title that's in all caps is longer than the same title in mixed case and even 80 characters can spill over to four lines in the right circumstances.

koji
koji reviewed Jun 21, 2023
View reviewed changes
app/src/pages/OnDeviceDisplay/ProtocolDetails/index.tsx
Copy link
Contributor

@koji koji Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L198 the time format was updated from yyyy -> yy recently. ex 2023 -> 23

https://www.figma.com/file/AoTLAYuWawlaWItB1umOjr/Release%3A-Opentrons-Flex-Touchscreen?type=design&node-id=7911-120967&t=a5E70qAYd84ya3n2-0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ewagoner
Seems that this PR needs to update the test for this time format change.

koji
koji reviewed Jun 21, 2023
View reviewed changes
app/src/pages/OnDeviceDisplay/ProtocolDetails/index.tsx
Comment on lines +175 to +181
<StyledText
as="p"
fontWeight={TYPOGRAPHY.fontWeightSemiBold}
>{`${i18n.format(t('author'), 'capitalize')}: `}</StyledText>
<StyledText as="p" fontWeight={TYPOGRAPHY.fontWeightSemiBold}>
{author}
</StyledText>
Copy link
Contributor

@koji koji Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally optional
probably we can do something like this

auther_name: "Auther: {{author}}"
<StyledText
  as="p"
  fontWeight={TYPOGRAPHY.fontWeightSemiBold}
>{`${i18n.format(t('author'), 'capitalize')}: `}
${i18n.format(t('author', {author: author}), 'capitalize')}
</StyledText>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this hard code the Author: portion instead of using the localized string?

koji
koji approved these changes Jun 21, 2023
View reviewed changes
Copy link
Contributor

@koji koji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple of comments. But otherwise, looks good to me.

@ewagoner ewagoner force-pushed the RAUT-451-protocol-detail-dqa-2 branch from 717db18 to 6e10d06 Compare June 21, 2023 19:10
@ewagoner ewagoner force-pushed the RAUT-451-protocol-detail-dqa-2 branch from 6e10d06 to f84f201 Compare June 21, 2023 20:39
@ewagoner ewagoner merged commit 6263d9a into edge Jun 21, 2023
@ewagoner ewagoner deleted the RAUT-451-protocol-detail-dqa-2 branch June 21, 2023 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koji koji koji approved these changes

@shlokamin shlokamin Awaiting requested review from shlokamin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ewagoner @koji