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: Adding start adornment to Chips #1058

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

JonnCharpentier
Copy link
Contributor

image

@JonnCharpentier JonnCharpentier force-pushed the sc-53838/adding-start-adorment branch from c97f852 to de4a8b0 Compare August 6, 2024 17:38
export function WithStartAdorment() {
return (
<div css={Css.wPx(200).$}>
<Chip startAdornment={<span css={Css.smBd.$}>K</span>} text="Kitchen" />
Copy link
Contributor

Choose a reason for hiding this comment

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

@JonnCharpentier this is fine, but I was thinking maybe we could just change text: string to text: ReactNode, similar to how title: ReactNode.

That way we could do like text=...snippet of html...

Can we change to that instead? It seems lighter weight / more flexible than a new prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the Chip that could work, but for the ToggleChipGroup we need the options.label to be a string so we can use it for aria-label.

@@ -109,7 +112,10 @@ function ToggleChip(props: ToggleChipProps) {
<VisuallyHidden>
<input {...inputProps} {...focusProps} />
</VisuallyHidden>
{label}
<div css={Css.df.gap1.$}>
{startAdornment}
Copy link
Contributor

Choose a reason for hiding this comment

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

@JonnCharpentier same push, I think label: ReactNode is a simpler/more extensible change to ToggleChip than a new startAdornment that isn't that complicated / value-adding...

(I.e. we do have things like TextField.startAdornment, but it's more intricate/complicated to place the adornment in "the right spot" of the input field, so it makes more sense for startAdornment to be a pattern there, vs. here where we could just as well send in some raw HTML.)

Copy link
Contributor Author

@JonnCharpentier JonnCharpentier Aug 6, 2024

Choose a reason for hiding this comment

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

@stephenh we need options.label to be a string, because we use it for aria-label.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see 🤔 ... that makes sense

@JonnCharpentier JonnCharpentier merged commit 54fa8f6 into main Aug 6, 2024
6 checks passed
@JonnCharpentier JonnCharpentier deleted the sc-53838/adding-start-adorment branch August 6, 2024 19:15
homebound-team-bot pushed a commit that referenced this pull request Aug 6, 2024
## [2.362.0](v2.361.0...v2.362.0) (2024-08-06)

### Features

* Adding start adornment to Chips ([#1058](#1058)) ([54fa8f6](54fa8f6))
@homebound-team-bot
Copy link

🎉 This PR is included in version 2.362.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants