fix: controlled tag component for selected prop #19228 #19231

sojinantony01 · 2025-04-24T14:04:40Z

Closes #19228

Controlled selectable tag component

Changelog

New

Controlled Tab component

Testing / Reviewing

https://stackblitz.com/edit/github-g93j9mdu?file=src%2FApp.jsx - component should be controllable with latest version

github-actions · 2025-04-24T14:05:01Z

All contributors have signed the DCO.
_{Posted by the DCO Assistant Lite bot.}

netlify · 2025-04-24T14:09:44Z

✅ Deploy Preview for v11-carbon-web-components ready!

Name	Link
🔨 Latest commit	`d62f987`
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-web-components/deploys/680a44fa49ad08000867d3da
😎 Deploy Preview	https://deploy-preview-19231--v11-carbon-web-components.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

sojinantony01 · 2025-04-24T14:10:42Z

recheck

netlify · 2025-04-24T14:14:42Z

✅ Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`d62f987`
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-react/deploys/680a44fa6fe63b0008c8fcab
😎 Deploy Preview	https://deploy-preview-19231--v11-carbon-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2025-04-24T14:15:12Z

✅ Deploy Preview for v11-carbon-web-components ready!

Name	Link
🔨 Latest commit	`abfd48a`
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-web-components/deploys/681da5197edce000080e9a62
😎 Deploy Preview	https://deploy-preview-19231--v11-carbon-web-components.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

sojinantony01 · 2025-04-24T14:22:11Z

I have read the DCO document and I hereby sign the DCO.

netlify · 2025-04-24T14:23:15Z

✅ Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`abfd48a`
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-react/deploys/681da519605bf0000779a6cb
😎 Deploy Preview	https://deploy-preview-19231--v11-carbon-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

tay1orjones

Thanks for writing a test to cover this! I made a more isolated repro for what it's worth: https://stackblitz.com/edit/github-g93j9mdu?file=src%2FApp.jsx

tay1orjones · 2025-04-24T19:38:13Z

packages/react/src/components/Tag/SelectableTag.tsx

+    useEffect(() => {
+      if (selected !== selectedTag) {
+        setSelectedTag(selected);
+      }
+    }, [selected]);


Does this need an effect?

https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes

I think we still need useEffect here. selected and onChange are optional props to the component, so if parent component not passing selected prop Tag component should handle is inside own state.
Do you agree? 🤔

if (selected !== selectedTag) { setSelectedTag(selected); }

If we add this to render, i think it is good to remove the state and use value directly from prop, but this case props needs to be mandatory.

Otherwise i can check if props passed or not in the condition

if (typeof selected == "boolean" && onChange && selected !== selectedTag) { setSelectedTag(selected); }

🤔.

@tay1orjones Please have a look

@sojinantony01 I forgot we have the useControllableState hook you can use for this.

carbon/packages/react/src/internal/useControllableState.ts

Line 38 in 15b90a4

export const useControllableState = <T>({

It'll handle the whole thing for you. We'll need to add a defaultSelected boolean prop to SelectableTag to pass to the hook. Probably the closest usage example you could follow is in Toggle:

carbon/packages/react/src/components/Toggle/Toggle.tsx

Lines 121 to 125 in a6eb265

const [checked, setChecked] = useControllableState({

value: toggled,

onChange: onToggle,

defaultValue: defaultToggled,

});

We'll want to make sure there's clear passing tests for both controlled and uncontrolled modes here too.

Thats really a useful hook. Thanks @tay1orjones.
I have updated the component using the useControllableState hook. we already have tests for uncontrolled state and i have added couple of tests for the controlled case - All passing.

And have manually tested the component using storybook.
Please have a look in the latest code.

codecov · 2025-04-24T19:59:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.78%. Comparing base (990010d) to head (abfd48a).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #19231      +/-   ##
==========================================
- Coverage   84.78%   84.78%   -0.01%     
==========================================
  Files         371      371              
  Lines       14411    14410       -1     
  Branches     4744     4692      -52     
==========================================
- Hits        12218    12217       -1     
- Misses       2044     2045       +1     
+ Partials      149      148       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

tay1orjones · 2025-05-05T16:56:50Z

packages/react/src/components/Tag/Tag-test.js

+    const selectableTag = container.firstChild;
+
+    await userEvent.click(selectableTag);


I'd prefer the tests to use screen.getByRole instead of container.firstChild and assert aria-pressed in addition to the toHaveClass assertions (since a user doesn't directly interact with the classnames), but it's not a blocker to merge.

Suggested change

const selectableTag = container.firstChild;

await userEvent.click(selectableTag);

await userEvent.click(screen.getByRole("button", {name: "Tag content"}));

Sure, Updated the tests.

tay1orjones

One minor thing, I think the default should be configurable

tay1orjones · 2025-05-05T17:00:34Z

packages/react/src/components/Tag/SelectableTag.tsx

+    const [selectedTag, setSelectedTag] = useControllableState({
+      value: selected,
+      onChange: onChange,
+      defaultValue: false,


Shouldn't this be configurable through a new defaultSelected prop, similar to what's done in other usages like Toggle and Tabs?

carbon/packages/react/src/components/Tabs/Tabs.tsx

Line 152 in a6eb265

defaultValue: defaultSelectedIndex,

It would have a default function param of false

Sure, Thanks @tay1orjones
Added a new defaultSelected prop and tests for same.

…19228

…sign-system#19228

tay1orjones

LGTM, thanks again!

…01/carbon into 19228-controlled-tag

riddhybansal

LGTM!!

…19228 (carbon-design-system#19231) * fix: controlled tag component for selected prop carbon-design-system#19228 * fix: fix tests for tag * fix: controlled tag component for selected prop carbon-design-system#19228 * fix: controlled tag added a default selected prop and tests carbon-design-system#19228 * fix: controlled tag added a default selected prop and tests carbon-design-system#19228 * fix: controlled tag added a default selected prop and tests carbon-design-system#19228 * fix: controlled tag fix tests carbon-design-system#19228 * fix: add profile * fix: add profile

* fix(icons): fix icons invisible in high contrast mode If the system has a dark high-contrast theme, then icons need to be displayed in white (or a light color), and vice-versa. Otherwise, icons become invisible. Importantly, this must happen regardless of the Carbon theme setting. Fixes #17147, #18830, #19015, #19023, #19140. * fix(icons): fix tooltip story Use proper color for icon, not background color. * fix(icons): remove vestigial high-contrast-mode('icon-fill') When you use standard tokens like $icon-primary you don't need it anymore. Note: I didn't convert $support-error and $support-warning usages to a system color. Previously the inline notification icon 🚫 was converted from red to ButtonText. Also $support-error and $support-warning usages in _list-box.scss. Also, inline-notification has a dodgy use of $black-100 that I didn't change. * fix(icons): handle weird values for fill Replace $text-primary with $icon-primary as they are the same. Likewise $tab-text-disabled --> $icon-disabled. Updated _theme.scss to handle the other weird fill settings. * fix(icons): fix comment * fix(icons): fix background colors * fix(icons): fix checkmark * fix(icons): remove unused imports of high-contrast-mode * fix: remove unexpected shadow from web-component button (#19347) * fix: controlled tag component for selected prop #19228 (#19231) * fix: controlled tag component for selected prop #19228 * fix: fix tests for tag * fix: controlled tag component for selected prop #19228 * fix: controlled tag added a default selected prop and tests #19228 * fix: controlled tag added a default selected prop and tests #19228 * fix: controlled tag added a default selected prop and tests #19228 * fix: controlled tag fix tests #19228 * fix: add profile * fix: add profile * fix(date-picker): clear flatpickr instance on reset (#19239) * fix(date-picker): clear flatpickr instance on reset * fix(date-picker): clear flatpickr instance on reset * fix(date-picker): update comments and storybook docs to clarify changes * fix(date-picker): update comments and storybook docs to clarify changes * Update packages/react/src/components/DatePicker/DatePicker.tsx Co-authored-by: kennylam <[email protected]> --------- Co-authored-by: kennylam <[email protected]> * fix: modal footer buttons were not fully visible in zoom to 400 (#19288) * fix: modal footer buttons were not fully visible in zoom to 400 * fix: added test story * fix: remove test story --------- Co-authored-by: Heloise Lui <[email protected]> * fix(icons): another inverted background * chore(icons): fix merge error --------- Co-authored-by: Heloise Lui <[email protected]> Co-authored-by: Sojin antony <[email protected]> Co-authored-by: Warren Blood <[email protected]> Co-authored-by: kennylam <[email protected]> Co-authored-by: Preeti Bansal <[email protected]>

sojinantony01 requested review from a team as code owners April 24, 2025 14:04

sojinantony01 requested review from Gururajj77 and alisonjoseph April 24, 2025 14:04

sojinantony01 force-pushed the 19228-controlled-tag branch from d62f987 to b024f41 Compare April 24, 2025 14:08

tay1orjones requested changes Apr 24, 2025

View reviewed changes

sojinantony01 requested a review from tay1orjones April 25, 2025 04:11

sojinantony01 force-pushed the 19228-controlled-tag branch from 986ed4a to ced22db Compare May 1, 2025 04:25

sojinantony01 requested a review from a team as a code owner May 1, 2025 04:25

sojinantony01 force-pushed the 19228-controlled-tag branch from ced22db to d66b4be Compare May 1, 2025 04:30

tay1orjones approved these changes May 5, 2025

View reviewed changes

tay1orjones requested changes May 5, 2025

View reviewed changes

sojinantony01 requested a review from tay1orjones May 6, 2025 05:57

sojinantony01 added 6 commits May 6, 2025 21:54

fix: controlled tag component for selected prop carbon-design-system#…

d0396dc

…19228

fix: fix tests for tag

a756377

fix: controlled tag component for selected prop carbon-design-system#…

e23b516

…19228

fix: controlled tag added a default selected prop and tests carbon-de…

0875082

…sign-system#19228

fix: controlled tag added a default selected prop and tests carbon-de…

b28b76f

…sign-system#19228

fix: controlled tag added a default selected prop and tests carbon-de…

58a5600

…sign-system#19228

sojinantony01 force-pushed the 19228-controlled-tag branch from 4915f6d to 58a5600 Compare May 6, 2025 16:24

sojinantony01 added 2 commits May 6, 2025 22:38

Merge branch 'main' into 19228-controlled-tag

32b969c

Merge branch 'main' into 19228-controlled-tag

c253577

tay1orjones approved these changes May 8, 2025

View reviewed changes

sojinantony01 added 4 commits May 8, 2025 21:09

fix: controlled tag fix tests carbon-design-system#19228

d935507

Merge branch '19228-controlled-tag' of https://github.com/sojinantony…

4cd785b

…01/carbon into 19228-controlled-tag

fix: add profile

067eee8

fix: add profile

a958ac1

tay1orjones added the status: one more review 👀 label May 8, 2025

Merge branch 'main' into 19228-controlled-tag

abfd48a

riddhybansal approved these changes May 9, 2025

View reviewed changes

riddhybansal enabled auto-merge May 9, 2025 14:03

riddhybansal added this pull request to the merge queue May 9, 2025

Merged via the queue into carbon-design-system:main with commit ab5a320 May 9, 2025
37 checks passed

sojinantony01 deleted the 19228-controlled-tag branch May 9, 2025 14:42

	const [checked, setChecked] = useControllableState({
	value: toggled,
	onChange: onToggle,
	defaultValue: defaultToggled,
	});

		const selectableTag = container.firstChild;

		await userEvent.click(selectableTag);

fix: controlled tag component for selected prop #19228 #19231

fix: controlled tag component for selected prop #19228 #19231

Uh oh!

Conversation

sojinantony01 commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Testing / Reviewing

Uh oh!

github-actions bot commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Apr 24, 2025

✅ Deploy Preview for v11-carbon-web-components ready!

Uh oh!

sojinantony01 commented Apr 24, 2025

Uh oh!

netlify bot commented Apr 24, 2025

✅ Deploy Preview for v11-carbon-react ready!

Uh oh!

netlify bot commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for v11-carbon-web-components ready!

Uh oh!

sojinantony01 commented Apr 24, 2025

Uh oh!

netlify bot commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for v11-carbon-react ready!

Uh oh!

tay1orjones left a comment

Choose a reason for hiding this comment

Uh oh!

tay1orjones Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sojinantony01 Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sojinantony01 Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

tay1orjones Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

sojinantony01 May 1, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tay1orjones May 5, 2025

Choose a reason for hiding this comment

Uh oh!

sojinantony01 May 6, 2025

Choose a reason for hiding this comment

Uh oh!

tay1orjones left a comment

Choose a reason for hiding this comment

Uh oh!

tay1orjones May 5, 2025

Choose a reason for hiding this comment

Uh oh!

sojinantony01 May 6, 2025

Choose a reason for hiding this comment

Uh oh!

tay1orjones left a comment

Choose a reason for hiding this comment

Uh oh!

riddhybansal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sojinantony01 commented Apr 24, 2025 •

edited

Loading

github-actions bot commented Apr 24, 2025 •

edited

Loading

netlify bot commented Apr 24, 2025 •

edited

Loading

netlify bot commented Apr 24, 2025 •

edited

Loading

tay1orjones Apr 24, 2025 •

edited

Loading

sojinantony01 Apr 25, 2025 •

edited

Loading

codecov bot commented Apr 24, 2025 •

edited

Loading