fix: [M3-7565] - Fix Radio size prop not affecting the radio button's dimensions #11242
Conversation
zaenab-akamai
changed the title
zaenab-akamai
changed the title
zaenab-akamai
requested review from
carrillo-erik and
hkhalil-akamai
and removed request for
a team
|
The ticket mentions removing the hardcoded size from the theme file and fixing any regressions... are we still planning to do that? (cc @coliu-akamai) |
| @@ -22,11 +22,18 @@ import { RadioIcon, RadioIconRadioed } from '../../assets/icons'; | |||
| - Give people control and align with their expectations | |||
| */ | |||
| export const Radio = (props: RadioProps) => { | |||
| const sizeInPixels: Record<string, string> = { | |||
| medium: '20px', | |||
| small: '16px', | |||
There was a problem hiding this comment.
let's add a large to this for 24px
There was a problem hiding this comment.
Hi @jaalah-akamai, I tried doing this.
MUI defines size (under RadioProps) with: size?: OverridableStringUnion<'small' | 'medium', RadioPropsSizeOverrides>; so we have just small and medium right now.
I considered extending RadioProps and adding on "large" but then we're spreading props into _Radio (MUI's original Radio) so it fails there (because _Radio doesn't know about large).
Would you happen to know a way to make this work? I might be missing something
|
I agree with @hkhalil-akamai we need to remove this: https://github.com/linode/manager/blob/develop/packages/ui/src/foundations/themes/light.ts#L1152-L1158 ultimately, but that will require some additional QA work in places we use icons |
| data-qa-radio={props.checked || false} | ||
| icon={<RadioIcon />} | ||
| icon={<RadioIcon height={iconDimension} width={iconDimension} />} |
There was a problem hiding this comment.
Ideally, we wrap any custom icons in SvgIcon MUI wrapper so we can use the built-in size prop instead of controlling the height/width manually. See: https://mui.com/material-ui/icons/#size
Alternatively we could also use createSvgIcon
There was a problem hiding this comment.
Along those lines, I wonder if we coud/should implement this in the theme rather than in the component. packages/ui/src/foundations/themes/light.ts
There was a problem hiding this comment.
This is done. I updated packages/ui/src/foundations/themes/light.ts to make the font sizes work. We could get these values from design tokens if they're present.
|
Coverage Report: ✅ |
Cloud Manager UI test results🎉 462 passing tests on test run #9 ↗︎
|
Cloud Manager E2E
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| Project |
Cloud Manager E2E
|
| Branch Review |
develop
|
| Run status |
|
| Run duration | 30m 25s |
| Commit |
|
| Committer | zaenab-akamai |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
3
|
Pending
|
2
|
Skipped
|
0
|
Passing
|
464
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/e2e/core/linodes/linode-config.spec.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Linode Config management > End-to-End > Clones a config |
Screenshots
Video
|
|
linode-config.spec.ts • 1 flaky test
| Test | Artifacts | |
|---|---|---|
| Linode Config management > End-to-End > Boots a config |
Screenshots
Video
|
|
clone-linode.spec.ts • 1 flaky test
| Test | Artifacts | |
|---|---|---|
| clone linode > can clone a Linode from Linode details page |
Screenshots
Video
|
|
update-linode-labels.spec.ts • 1 flaky test
| Test | Artifacts | |
|---|---|---|
| update linode label > updates a linode label from details page |
Screenshots
Video
|
|
Description 📝
We realized this issue while working on the Radio storybook: changing the size prop of the radio did not actually affect the radio's appearance.
I noticed the size of the radio buttons was always 25px - which was coming from the width and height set in
radio.svgandradioRadioed.svg. The font-size being set had no effect on the radio's dimensions.Changes 🔄
heightandwidthtoRadioIconRadioedandRadioIconbased on the size prop's valueTarget release date 🗓️
NA
Preview 📷
How to test 🧪
Reproduction steps
Verification steps
NOTE: It's possible that updating the medium radio size to 20px could cause alignment issues on pages where radios are used. We'll need to test this
As an Author I have considered 🤔
Check all that apply