-
Notifications
You must be signed in to change notification settings - Fork 462
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(Chip): new component #886
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
@benjamincanac let me know your thoughts on this component. I've been debating the name of it with myself π
. I always feel like My thought would be to name this |
We'll surely not be renaming the |
Sounds good! I haven't removed the |
I'm not sure yet, maybe we can let the prop indeed to prevent a breaking change. We'll remove it in the next major! |
Sounds like a good idea to me! |
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.
I've made a few changes and updates the size
to be the same as the avatar.chip
mainly for the padding otherwise it doesn't look good with text on small sizes. Any reason to have changed it?
Also, what's the point of the translate
and the margin
? If you delete both it comes down to the same thing or am I missing something?
@benjamincanac No reason to have changed the size, probably just oversight on my part. For the If we add the This is because the Hopefully that makes sense or maybe I misunderstood the question π |
It does! But why add the |
Yep you're exactly right! Thanks for pointing that out, I don't think I would have noticed that π I'll make that change to simplify it |
Also, we need to handle the safelist for this new component. I can commit to the PR if needed π |
Feel free! I'm not exactly sure how that works, so I'll check out the change you make so I can learn for next time π Thank you! |
Went ahead and added it. Let me know if there's more I need to add for the safelisting π |
@connerblanton That was it π Thanks! |
@benjamincanac Upon checking the naming convention, I'd like to suggest a different perspective. The term "chip" initially seemed a bit counterintuitive to me, and I initially thought of it as a badge. It might be helpful to consider the existing badge component as an "inline badge," and this new addition could potentially be termed a "floating badge." This suggestion is not intended to undermine the current naming but rather to offer an alternative viewpoint that might align more closely with the component's functionality and appearance. Since this is a recent release, would it be possible to reconsider the naming to perhaps "badge"? I appreciate your consideration of this suggestion. |
The |
π Linked issue
#572
β Type of change
π Description
New Chip component that can be wrapped around any other component
Resolves #572
π Checklist