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.

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

feat(odd): radio button component #12539

Merged
merged 6 commits into from
Apr 21, 2023
Merged

feat(odd): radio button component #12539

merged 6 commits into from
Apr 21, 2023

Conversation

ewagoner
Copy link
Contributor

@ewagoner ewagoner commented Apr 21, 2023

Overview

This PR adds a radio button component for use on the ODD.

Design: https://www.figma.com/file/OIdG64Q5cgvEw82ish5Eee/Design-System-(Flex)?node-id=1108-58731&t=40X6Wfql6XIyqtAl-0

Closes RAUT-366

Test Plan

I created both unit tests and storybook stories for the different sizes and states of the radio button.

Changelog

  • Created radio button component
  • Created unit tests and storybook stories
  • Converted sleep settings screen to use new component

Review requests

Verify storybook matches design and that sleep settings screen still functions and matches design.

Risk assessment

Low. The only existing screen using this component is the sleep settings.

@ewagoner ewagoner requested review from koji and a team April 21, 2023 05:35
@ewagoner ewagoner requested a review from a team as a code owner April 21, 2023 05:35
@codecov
Copy link

codecov bot commented Apr 21, 2023

Codecov Report

Merging #12539 (63acbe3) into edge (5197f97) will decrease coverage by 0.32%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #12539      +/-   ##
==========================================
- Coverage   73.68%   73.37%   -0.32%     
==========================================
  Files        2262     1503     -759     
  Lines       62125    49168   -12957     
  Branches     6580     2997    -3583     
==========================================
- Hits        45779    36075    -9704     
+ Misses      14782    12636    -2146     
+ Partials     1564      457    -1107     
Flag Coverage Δ
app 46.77% <ø> (-25.61%) ⬇️

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

see 759 files with indirect coverage changes

@ewagoner ewagoner force-pushed the RAUT-366-odd-radio-button branch from ab39eb6 to 81c8d17 Compare April 21, 2023 14:07
${disabled && disabledButtonStyles}

&:focus-visible {
box-shadow: 0 0 0 ${SPACING.spacingS} ${COLORS.fundamentalsFocus};
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed that 👇
https://www.figma.com/file/OIdG64Q5cgvEw82ish5Eee/Design-System-(Flex)?node-id=1224-61431&t=40X6Wfql6XIyqtAl-0

export const ODD_FOCUS_VISIBLE = `0 0 0 ${SPACING.spacing1} ${COLORS.fundamentalsFocus}`

But still not sure that why Mel sees the different visual 🤔

Suggested change
box-shadow: 0 0 0 ${SPACING.spacingS} ${COLORS.fundamentalsFocus};
box-shadow: 0 0 0 ${SPACING.spacing2} ${COLORS.fundamentalsFocus};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's odd. All the button focus borders in figma are 4px, not 2px. I'll change the constant to 0.25rem and use that.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks.
But still not sure why Mel saw the difference between SmallButton and CardButton.
Actually I was tunneling my localhost:6060 via ngrok since s3 has a cache issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty sure she was looking at MediumButton, not SmallButton. It had the issue she was seeing, and I fixed that last night.

buttonLabel={radio.label}
buttonValue={radio.value}
onChange={handleChange}
isSelected={radio.value === sleepMs}
Copy link
Contributor

Choose a reason for hiding this comment

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

The current size is 68px but the height in Figma is 84px (5.25rem)
https://www.figma.com/file/AoTLAYuWawlaWItB1umOjr/Release%3A-Opentrons-Flex-Touchscreen-Designs?node-id=7808-158040&t=YUW2iGF5OAks9tpZ-0

Suggested change
isSelected={radio.value === sleepMs}
isSelected={radio.value === sleepMs}
height="5.25rem"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figma says the height should hug, so we do not want a fixed height. In my storybook, the large button is 84px and the small is 68px, same as the designs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, got it.
I think the issue will be solved by adding radioButtonType='large' or setting radioBtuttonType = 'large'

Screenshot 2023-04-21 at 11 28 29 AM

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.

Tested on my dev kit.
Sleep worked as expected and the height issue was solved.
Thank you for adding 🔘 component

@ewagoner ewagoner merged commit 34eec84 into edge Apr 21, 2023
@ewagoner ewagoner deleted the RAUT-366-odd-radio-button branch April 21, 2023 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants