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

TagItem and TagList components #212

Merged
merged 4 commits into from
Nov 19, 2020
Merged

TagItem and TagList components #212

merged 4 commits into from
Nov 19, 2020

Conversation

winkerVSbecks
Copy link
Collaborator

@winkerVSbecks winkerVSbecks commented Oct 15, 2020

Not sure if this is the best API for the TagList

Copy link
Collaborator

@kylesuss kylesuss left a comment

Choose a reason for hiding this comment

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

I'm good on this, pending any API comments from others 👍

src/components/tag/TagItem.stories.js Outdated Show resolved Hide resolved
@winkerVSbecks
Copy link
Collaborator Author

@domyen @kylesuss updated with requested changes. I ended up creating a TagLink to handle links. TagItem is the non-interactive version.

@kylesuss
Copy link
Collaborator

@winkerVSbecks nice, sounds and looks good to me.

Out of curiosity, Varun, do you think all components in this design system should export themselves as a forwardRef? Is that a good goal to work toward? TBH, it feels weird to me to do that but also annoying when you finally end up needing that ref and have to go back and add it.

@winkerVSbecks
Copy link
Collaborator Author

Yea, that seems to be the pattern most DS adopt. Makes sense since they are mostly low level components. That said, typing those components is a pain.

I don't plan on rewriting everything. We can do that for new components and update old components as and when necessary.

@jonniebigodes
Copy link

@winkerVSbecks just checked the latest commit you've made and it looks really great. I'm good with it as well 👍

src/components/tag/TagItem.stories.tsx Outdated Show resolved Hide resolved
src/components/tag/TagLink.tsx Outdated Show resolved Hide resolved
@domyen
Copy link
Member

domyen commented Oct 20, 2020

Heads up, I'm leaving specific UI comments in Chromatic

@winkerVSbecks
Copy link
Collaborator Author

@domyen updated with changes you requested.

@winkerVSbecks winkerVSbecks added minor Increment the minor version when merged and removed enhancement New feature or request labels Nov 18, 2020
@winkerVSbecks winkerVSbecks merged commit 9c9fd1b into master Nov 19, 2020
@kylesuss
Copy link
Collaborator

🚀 PR was released in v5.2.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants