-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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(react-tag): Added selected
state for Tag and InteractionTag
#33804
base: master
Are you sure you want to change the base?
feat(react-tag): Added selected
state for Tag and InteractionTag
#33804
Conversation
...es/react-components/react-tags/library/src/components/InteractionTag/InteractionTag.types.ts
Show resolved
Hide resolved
58d7bd3
to
707bd31
Compare
📊 Bundle size reportUnchanged fixtures
|
...nents/react-tags/library/src/components/InteractionTagPrimary/InteractionTagPrimary.types.ts
Outdated
Show resolved
Hide resolved
1d1d04f
to
b5b5bb9
Compare
b5b5bb9
to
306c796
Compare
Pull request demo site: URL |
d746f72
to
1ccba12
Compare
Hi there, this PR contains only style change for the selected Tag/InteractionTag.
Do you plan to have follow up PR to address the above? |
There's mention that it's only the first part of this feature. There should be also controllable state. They wanted this feature urgent but considering how long the PR review takes it's not that urgent after all X) |
I see. Could you add a warning in the selected Tag/InteractionTag story, so it's clear to the users that this feature is only visual currently? And the warning can be removed when the feature is completed |
Or if there's a better way to release 'preview' features like that, I'm all for it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, the only thing to watch out for is what Amber said. While I don't think there's any issues with diving it the way you're doing it (style changes + prop addition -> logic + semantics), I leave it up to y'all if this should be changed. If so, I would suggest adding that logic in this PR, I don't think it would add too much complexity.
...es/react-components/react-tags/library/src/components/InteractionTag/InteractionTag.types.ts
Outdated
Show resolved
Hide resolved
packages/react-components/react-tags/library/src/components/Tag/Tag.types.ts
Outdated
Show resolved
Hide resolved
…teractionTag/InteractionTag.types.ts Co-authored-by: Esteban Munoz Facusse <[email protected]>
…g/Tag.types.ts Co-authored-by: Esteban Munoz Facusse <[email protected]>
New Behavior
Added

selected
state for Tag and InteractionTagVariants of selected state (for testing)

Related Issue(s)
First part of this feature #33738