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

feat: Adding start adornment to Chips #1058

Merged
merged 2 commits into from
Aug 6, 2024
Merged

feat: Adding start adornment to Chips #1058

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de4a8b0
feat: Adding start adorment to Chips
JonnCh Aug 6, 2024
e037c5a
PR feedback
JonnCh Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/components/Chip.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,15 @@ export function WithIcon() {
</div>
);
}

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.

<Chip
startAdornment={<span css={Css.smBd.$}>A</span>}
text="A very long chip name that will eventually truncate"
/>
</div>
);
}
4 changes: 3 additions & 1 deletion src/components/Chip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export const ChipTypes: Record<ChipType, ChipType> = {
export interface ChipProps<X> {
text: string;
title?: ReactNode;
startAdornment?: ReactNode;
xss?: X;
type?: ChipType;
compact?: boolean;
Expand All @@ -34,7 +35,7 @@ export function Chip<X extends Only<Xss<Margin | "color" | "backgroundColor">, X
...props
}: ChipProps<X>) {
const { fieldProps } = usePresentationContext();
const { text, title, xss = {}, compact = fieldProps?.compact, icon } = props;
const { text, title, xss = {}, compact = fieldProps?.compact, icon, startAdornment } = props;
const tid = useTestIds(props, "chip");

return maybeTooltip({
Expand All @@ -50,6 +51,7 @@ export function Chip<X extends Only<Xss<Margin | "color" | "backgroundColor">, X
{...tid}
>
{icon && <Icon icon={icon} inc={2} xss={Css.fs0.$} />}
{startAdornment}
<span css={Css.lineClamp1.wbba.$}>{text}</span>
</span>
),
Expand Down
8 changes: 8 additions & 0 deletions src/inputs/ToggleChipGroup.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@ export function ToggleChipGroups() {
values={selectedValues}
onChange={setSelectedValues}
/>
<p css={Css.mt2.mb2.$}>Start Adornment:</p>
<ToggleChipGroup
label="Select Markets"
labelStyle="left"
options={options.map((o) => ({ ...o, startAdornment: <span css={Css.smBd.$}>{o.label.slice(0, 2)}</span> }))}
values={selectedValues}
onChange={setSelectedValues}
/>
</>
);
}
10 changes: 8 additions & 2 deletions src/inputs/ToggleChipGroup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ type ToggleChipXss = Xss<"color" | "backgroundColor">;
type ToggleChipItemProps = {
label: string;
value: string;
startAdornment?: ReactNode;
/**
* Whether the interactive element is disabled.
*
Expand Down Expand Up @@ -55,6 +56,7 @@ export function ToggleChipGroup(props: ToggleChipGroupProps) {
selected={state.value.includes(o.value)}
label={o.label}
disabled={o.disabled}
startAdornment={o.startAdornment}
xss={xss}
{...tid[o.value]}
/>
Expand All @@ -75,11 +77,12 @@ interface ToggleChipProps {
* If a ReactNode, it's treated as a "disabled reason" that's shown in a tooltip.
*/
disabled?: boolean | ReactNode;
startAdornment?: ReactNode;
xss?: ToggleChipXss;
}

function ToggleChip(props: ToggleChipProps) {
const { label, value, groupState, selected: isSelected, disabled = false, xss, ...others } = props;
const { label, value, groupState, selected: isSelected, disabled = false, startAdornment, xss, ...others } = props;
const isDisabled = !!disabled;
const ref = useRef(null);
const { inputProps } = useCheckboxGroupItem({ value, "aria-label": label, isDisabled }, groupState, ref);
Expand Down Expand Up @@ -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 •
edited
Loading

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

{label}
</div>
</label>
),
});
Expand Down
Loading