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

App added new styling to protocol landing page #10715

Closed
wants to merge 14 commits into from
Closed

App added new styling to protocol landing page #10715

wants to merge 14 commits into from

Conversation

aptrons22
Copy link
Contributor

@aptrons22 aptrons22 commented Jun 10, 2022 •
edited
Loading

Overview

The PR will fix #10488

  • (1) Missing the enabled state (no fill background), and needs a hover state linked up.
  • (2) Remove the white fill from the button
  • (3) When I press on the button, i get the orange outline. The orange outline is only the focus state and should only appear when a user tabs through all the links using 'tab' on their keyboard. So remove this please! (Already Done)
  • (4) update 'sort by' text to our darkGreyEnabled
  • (5) Looks like we're missing the hover state for the protocol cards. Check the design system for info
  • (7) Update this text across all cards to our darkGreyEnabled
  • (8) The columns should have a min-width of 170px so regardless of what content is in them, the columns will always align. These columns should be responsive when the user shrinks the viewport.
  • (9) Update both of these so it's our label text in darkGreyEnabled. No uppercase here
  • (10) Update text should align to the bottom of the other containers to the left.
  • (11) Update to be our p-semibold, currently it's bold
  • (12) Update to our label-semibold styling.
  • (13) Double check the colour of the text and the icon. They should be the same
  • (14) Update the spacing here between header and text below to .25 rem

Screen Shot 2022-06-06 at 2 02 01 PM

Changelog

  • The following list above has been completed.
  • Styling of icons has been modified
  • Styling of text colors
  • Icon behaviors has been revised

Review requests

Low

Risk assessment

@aptrons22 aptrons22 self-assigned this Jun 10, 2022
@aptrons22 aptrons22 requested a review from koji June 10, 2022 03:40
@codecov
Copy link

codecov bot commented Jun 10, 2022 •
edited
Loading

Codecov Report

Merging #10715 (86bf200) into edge (b06ba18) will increase coverage by 0.39%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #10715      +/-   ##
==========================================
+ Coverage   73.83%   74.23%   +0.39%     
==========================================
  Files        2139     2162      +23     
  Lines       57443    59251    +1808     
  Branches     5778     6689     +911     
==========================================
+ Hits        42414    43985    +1571     
- Misses      13822    13863      +41     
- Partials     1207     1403     +196
Flag Coverage Δ
app 72.16% <0.00%> (+0.80%) ⬆️
components 52.78% <100.00%> (+0.03%) ⬆️
hardware-testing ?
labware-library 49.74% <ø> (ø)
protocol-designer 46.48% <ø> (+0.89%) ⬆️
react-api-client 82.68% <ø> (ø)
step-generation 86.48% <ø> (ø)

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

Impacted Files Coverage Δ
app/src/atoms/MenuList/OverflowBtn.tsx 100.00% <ø> (ø)
app/src/molecules/DeckThumbnail/index.tsx 88.23% <ø> (ø)
...src/organisms/ProtocolsLanding/EmptyStateLinks.tsx 100.00% <ø> (ø)
...pp/src/organisms/ProtocolsLanding/ProtocolCard.tsx 0.00% <0.00%> (ø)
...pp/src/organisms/ProtocolsLanding/ProtocolList.tsx 0.00% <ø> (ø)
...rganisms/ProtocolsLanding/ProtocolOverflowMenu.tsx 86.66% <ø> (ø)
components/src/ui-style-constants/typography.ts 100.00% <100.00%> (ø)
shared-data/js/helpers/index.ts 56.89% <0.00%> (-13.89%) ⬇️
app/src/organisms/Devices/RobotStatusBanner.tsx 76.47% <0.00%> (-12.42%) ⬇️
...ices/RobotSettings/AdvancedTab/Troubleshooting.tsx 90.00% <0.00%> (-10.00%) ⬇️
... and 144 more

koji
koji reviewed Jun 10, 2022
View reviewed changes
app/src/organisms/ProtocolsLanding/EmptyStateLinks.tsx
<Icon
name={'open-in-new'}
marginLeft={SPACING.spacing2}
size="10px"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is weird since you updated this file before and edge doesn't have this size prop.
Could you check that your working branch is checked out from edge

koji
koji reviewed Jun 10, 2022
View reviewed changes
app/src/organisms/ProtocolsLanding/EmptyStateLinks.tsx
<Link
fontSize={FONT_SIZE_CAPTION}
color={C_MED_GRAY}
css={TYPOGRAPHY.darkLinkPSemiBold}
color={COLORS.darkBlack}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary since darkLinkPSemiBold has color property.

koji
koji reviewed Jun 10, 2022
View reviewed changes
app/src/organisms/ProtocolsLanding/EmptyStateLinks.tsx
/>
</Link>
<Link
fontSize={FONT_SIZE_CAPTION}
color={C_MED_GRAY}
css={TYPOGRAPHY.darkLinkPSemiBold}
color={COLORS.darkBlack}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary since darkLinkPSemiBold has color property.

koji
koji reviewed Jun 10, 2022
View reviewed changes
app/src/organisms/ProtocolsLanding/EmptyStateLinks.tsx
@@ -40,37 +40,48 @@ export function EmptyStateLinks(props: Props): JSX.Element | null {
<Flex justifyContent={JUSTIFY_START} flexDirection={DIRECTION_ROW}>
<Link
fontSize={FONT_SIZE_CAPTION}
color={C_MED_GRAY}
css={TYPOGRAPHY.darkLinkPSemiBold}
color={COLORS.darkBlack}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary since darkLinkPSemiBold has color property.

koji
koji reviewed Jun 10, 2022
View reviewed changes
app/src/organisms/ProtocolsLanding/EmptyStateLinks.tsx
href={PROTOCOL_LIBRARY_URL}
id={'EmptyStateLinks_protocolLibraryButton'}
marginRight={SPACING.spacing3}
opacity="0.7"
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend you to add opacity to darkLinkPSemiBold then you don't need to add opacity to each Link

koji
koji reviewed Jun 10, 2022
View reviewed changes
app/src/organisms/ProtocolsLanding/EmptyStateLinks.tsx
external
>
{t('browse_protocol_library')}
<Icon
name={'open-in-new'}
marginLeft={SPACING.spacing2}
size="10px"
size="0.5rem"
opacity="0.7"
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably don't need to add opacity to Icon if Link has opacity

koji
koji reviewed Jun 10, 2022
View reviewed changes
components/src/ui-style-constants/typography.ts
@@ -120,3 +120,14 @@ export const linkPSemiBold = css`
opacity: 70%;
}
`

export const darkLinkPSemiBold = css`
font-size: ${fontSizeP};
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use linkPSemiBold for L125-L127 `.

koji
koji reviewed Jun 10, 2022
View reviewed changes
app/src/atoms/MenuList/OverflowBtn.tsx
@@ -7,20 +7,25 @@ const overflowButtonStyles = css`
max-height: ${SPACING.spacing6};

&:hover {
background-color: ${COLORS.medGrey};
background-color: ${COLORS.darkGreyHover};
Copy link
Contributor

Choose a reason for hiding this comment

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

Jethary has been working on overflow-btn style, so you wouldn't need to work on this.

@aptrons22 aptrons22 marked this pull request as ready for review June 10, 2022 17:05
@aptrons22 aptrons22 requested a review from a team as a code owner June 10, 2022 17:05
@aptrons22 aptrons22 requested a review from b-cooper June 10, 2022 17:06
@koji koji requested review from mmencarelli and removed request for a team June 10, 2022 20:38
@aptrons22 aptrons22 closed this Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koji koji koji left review comments

Assignees

@aptrons22 aptrons22

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@aptrons22 @koji