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: Add Badge component #66

Merged
merged 14 commits into from
Sep 25, 2023
Merged

feat: Add Badge component #66

merged 14 commits into from
Sep 25, 2023

Conversation

magnuh
Copy link
Collaborator

@magnuh magnuh commented Sep 11, 2023

Figma: https://www.figma.com/file/8P1JQsd82b93gQ6K3igO2p/Warp---Components?type=design&node-id=381-42423&mode=design&t=fRLDoRSCmQEMZ7Si-0

This PR uses the changes published through the following PR:s:
warp-ds/drive#156 (@warp-ds/uno v.1.1.0)
warp-ds/css#48 (@warp-ds/css v1.1.0)

@magnuh magnuh requested a review from a team September 11, 2023 11:39
@magnuh magnuh self-assigned this Sep 11, 2023
@magnuh magnuh marked this pull request as draft September 11, 2023 11:41
@magnuh magnuh marked this pull request as ready for review September 20, 2023 09:18
@@ -36,6 +36,7 @@ export const sidebarConfig = [
title: 'Layout',
Copy link
Contributor

Choose a reason for hiding this comment

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

I have just found a component in this package that looks very much like our Badge. It's called Tag and is listed under "Communication" section.

  1. Would it make sense to put Badge under "Communication", too?
  2. Should we maybe add some deprecation message to use Badge instead of Tag?
Screenshot 2023-09-20 at 12 30 13 Screenshot 2023-09-20 at 12 31 28

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Yes, that makes sense. 👍

  2. I have no idea... this is what Figma says:

Badges and tags are visually similar, but tags are interactive and badges are not...

It is also called "Ribbon" and there are some implementations out there that uses those component-classes for custom made "badges"... Makes me want to just drop everything when it's not the first time that you do something and try to understand the mess and asks for input... then finish it, the way you understand it... only to be told afterwards that it is "wrong". 😞

Copy link
Contributor

@BalbinaK BalbinaK Sep 20, 2023

Choose a reason for hiding this comment

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

I'm sorry to not have spotted it sooner :( It has also been confusing to me last time I looked at it - one and the same component being called ribbon, badge and tag. I think it all came from the fact that different people were developing Vue and React component libraries in Fabric. I tried aligning it with @adidick a few months back but we didn't land on anything, just got more confused and blocked with different approaches to the naming and so on 😅 I thought this was discussed again between you two and that you landed on the "Badge" as the way to go. If that's the case then that's great because we need that component everywhere, with just 1 name 😄 The only thing that's left is removing the deprecated Tag in Vue, but we could do that some time in the future. I only thought we could make a note/suggestion that Tag is deprecated in favour of Badge.

Badges and tags are visually similar, but tags are interactive and badges are not...

Where did you find this description? I can't see that the two are different in this way, at least based on the implementation. But maybe I'm reading vue code wrong 👀

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But no, you are not reading the vue code wrong... it seems to be doing the same thing as badge.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds to me like good news 🙂 We wouldn't be able to rename Tag anyway due to backward compatibility (who knows who uses that except us in the vue docs). I think we're free to only display Badge in the list of components both in the tech docs and in these vue docs if that helps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Anna did a code search for the Tag component and couldn't find anything. The component is broken and would, as an example, not get base styling if not adding a primary prop to it. Which doesn't make sense. I removed it from the sidebar in the internal docs in this repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wonderful! 🙌

Comment on lines 27 to 30
[ccBadge.positionTL]: props.position === 'tl',
[ccBadge.positionTR]: props.position === 'tr',
[ccBadge.positionBR]: props.position === 'br',
[ccBadge.positionBL]: props.position === 'bl',
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we know that Badge was previously a Tag, and that component had topLeft, topRight, bottomLeft and bottomRight in its API, would it make sense to use those? It always takes me a second to understand which position tl, tr, br and bl stand for, but that's just me 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the tag component they are separate properties, which means that you can add both bottomLeft and topRight. That is confusing to me. But sure, changing to position="top-left|top-right|bottom-right|bottom-left" would be easier to understand. 👍

Copy link
Contributor

@BalbinaK BalbinaK Sep 20, 2023

Choose a reason for hiding this comment

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

In the tag component they are separate properties, which means that you can add both bottomLeft and topRight.

I don't think those were ever intended to be used together. I see there are other examples of such boolean attributes in Vue components that were not really supposed to be used together either, but it was technically that was possible 😊

package.json Outdated
@@ -62,7 +62,7 @@
"happy-dom": "^9.20.3",
"semantic-release": "^21.1.1",
"shiki": "^0.14.3",
"unocss": "^0.55.3",
"unocss": "^0.56.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would maybe update unocss separately next time to avoid it breaking something in our packages. It is still v0 so minor updates could in theory be problematic for us :) It did happen when updating to v0.55 before. It resulted in @warp-ds/uno not working correctly, although all worked fine within component repos.

Comment on lines 6 to 15
const badgeTypes = [
{ name: 'Undefined', radio },
{ name: 'neutral', radio },
{ name: 'info', radio },
{ name: 'positive', radio },
{ name: 'warning', radio },
{ name: 'negative', radio },
{ name: 'disabled', radio },
{ name: 'notification', radio },
{ name: 'price', radio },
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that when it comes to styling/color of a component, a variant name is often used instead of type (see Button or Alert). I think it's the same way designers refer to different types of components.
Not that variant would be my first pick when dealing with a component type but we couldn't use type in e.g. Button for a similar purpose, because that was a reserved attribute there. Then we had to go with variant.
I'm saying all that to hear what you think about aligning this sort of API. What we decide today might be difficult to rename later, so I thought it's better to ask now.

Copy link
Collaborator Author

@magnuh magnuh Sep 20, 2023

Choose a reason for hiding this comment

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

Sure I can change it to variant. 👍 That's better when, as you say, it is not conflicting with html type attributes. It also seems to be how it is named in Figma (at least for this component).

@magnuh magnuh merged commit cd1a28c into next Sep 25, 2023
3 checks passed
@magnuh magnuh deleted the add-badge-component branch September 25, 2023 13:34
github-actions bot pushed a commit that referenced this pull request Sep 25, 2023
# [1.1.0-next.1](v1.0.1...v1.1.0-next.1) (2023-09-25)

### Features

* Add Badge component ([#66](#66)) ([cd1a28c](cd1a28c))
github-actions bot pushed a commit that referenced this pull request Oct 9, 2023
# [1.1.0](v1.0.1...v1.1.0) (2023-10-09)

### Features

* Add Badge component ([#66](#66)) ([cd1a28c](cd1a28c))
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.

4 participants