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(radiogroup): add labelDescription prop to radio items #514

Merged
merged 4 commits into from
Jan 21, 2022

Conversation

akornmeier
Copy link
Contributor

closes: #513

@github-actions
Copy link
Contributor

Preview branch generated at https://feat-radio-item-description.d1gko6en628vir.amplifyapp.com

@akornmeier akornmeier requested a review from scurker January 17, 2022 22:23
okry
okry previously approved these changes Jan 17, 2022
font-size: var(--text-size-small);
font-weight: var(--font-weight-normal);
line-height: 1;
margin: 0 0 0 30px;
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: This should probably match the computed width of the icon, unfortunately we don't have icon size anywhere as a variable so that is something that we should probably refactor in cauldron somewhere else. At a minimum, we could use calc:

margin-right: calc(24px + 2px + var(--space-half));
  • 24px - icon size
  • 2px - borders (left and right)
  • var(--space-half) matching the natural spacing between the input and the text

I don't recall why we put an invisible border around the input though 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair enough, i was looking for an icon size var and didn't see one. I will update to use the proposed value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might want to pass this by @awpearlm as well if he's not already aware

@awpearlm - Had a CSS change for the desc text that @bobbyomari will be working on. Once he has a design, he will link it to the original issue #513 and I will make the update. Aaron also was interested in adding this prop to the checkbox element as well. I will create a new issue for that in GH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewed the changes with @awpearlm, he approves.

@scurker
Copy link
Member

scurker commented Jan 18, 2022

Might want to pass this by @awpearlm as well if he's not already aware

@akornmeier akornmeier requested a review from awpearlm January 18, 2022 20:18
@akornmeier akornmeier merged commit 78d6a74 into develop Jan 21, 2022
@akornmeier akornmeier deleted the feat/radio-item-description branch January 21, 2022 15:38
@github-actions
Copy link
Contributor

Preview branch generated at https://feat-radio-item-description.d1gko6en628vir.amplifyapp.com

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.

Add an optional labelDescription prop to items in Radiogroup component
3 participants