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

feat(Chip): new component #886

Merged
merged 17 commits into from
Nov 19, 2023
Merged

feat(Chip): new component #886

merged 17 commits into from
Nov 19, 2023

Conversation

connerblanton
Copy link
Contributor

@connerblanton connerblanton commented Oct 28, 2023

πŸ”— Linked issue

#572

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

New Chip component that can be wrapped around any other component

Resolves #572

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@vercel
Copy link

vercel bot commented Oct 28, 2023

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated (UTC)
ui βœ… Ready (Inspect) Visit Preview Nov 18, 2023 4:05pm

@connerblanton
Copy link
Contributor Author

@benjamincanac let me know your thoughts on this component.

I've been debating the name of it with myself πŸ˜…. I always feel like Chip and Badge can be used interchangeably. Looking around at other UI libraries (ShadCN Badge, Vuetify Chip, PrimeVue Badge, PrimeVue Chip) this seems more like a Badge than a Chip. However, that would be a big breaking change to rename our current Badge component.

My thought would be to name this Indicator. I feel like that would remove confusion of Chip vs Badge. Interested to see what you or anyone else thinks on the naming of it. Thanks!

Copy link
Member

We'll surely not be renaming the Badge component and as this PR will create a breaking change for the Avatar already by removing the chip prop, I'd stick to calling this component Chip I guess.

@connerblanton
Copy link
Contributor Author

Sounds good! I haven't removed the chip prop for Avatar because I didn't want to introduce a breaking change for it. But I can do that if you want me to

Copy link
Member

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!

@connerblanton
Copy link
Contributor Author

Sounds like a good idea to me!

Copy link
Member

@benjamincanac benjamincanac left a 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?

@connerblanton
Copy link
Contributor Author

@benjamincanac No reason to have changed the size, probably just oversight on my part.

For the translate and the margin, those are necessary for when you are using elements other than an avatar. Without them, it still looks good for the avatar, but the button has the Chip fully inside of it
Screenshot 2023-11-17 at 8 57 38 AM

If we add the translate and margin back in, then we get the Chip positioned nicely on the button. But you can see the avatar is now off.
Screenshot 2023-11-17 at 8 59 43 AM

This is because the avatar is rounded so this is why we have the inset prop. We need to add some margin to get the Chip to position nicely on the rounded component
Screenshot 2023-11-17 at 9 03 00 AM

Hopefully that makes sense or maybe I misunderstood the question πŸ˜€

Copy link
Member

It does! But why add the margin when inset prop is present instead of just removing the translate? Wouldn't it be simpler?

@connerblanton
Copy link
Contributor Author

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

Copy link
Member

Also, we need to handle the safelist for this new component. I can commit to the PR if needed 😊

@connerblanton
Copy link
Contributor Author

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!

@connerblanton
Copy link
Contributor Author

Went ahead and added it. Let me know if there's more I need to add for the safelisting 😊

@benjamincanac
Copy link
Member

@connerblanton That was it 😊 Thanks!

@benjamincanac benjamincanac merged commit d4f1b5e into nuxt:dev Nov 19, 2023
2 checks passed
@jd-solanki
Copy link

@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.

Copy link
Member

The Badge component has been there since the beginning, I don't want to introduce that big of a breaking change for naming honestly. We'll consider this on the next major release.

@jd-solanki jd-solanki mentioned this pull request Jun 26, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Add Chip to UButton
3 participants