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 #703

Merged
merged 7 commits into from
Nov 1, 2022
Merged

feat: Add Badge component #703

merged 7 commits into from
Nov 1, 2022

Conversation

frankieyan
Copy link
Member

@frankieyan frankieyan commented Oct 28, 2022

Short description

This adds a badge component with 3 variants available:

image

Figma: https://www.figma.com/file/LYlWNzvhMDh907l07mPPQk/Product-Library---Web?node-id=9038%3A280583

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • Updated all static build artifacts (npm run build-all)

Versioning

New feature: minor

@frankieyan frankieyan requested review from a team and pawelgrimm and removed request for a team October 28, 2022 19:15
Copy link
Contributor

@pawelgrimm pawelgrimm left a comment

Choose a reason for hiding this comment

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

LGTM! I left some minor comments for you.

Will you be creating a new minor version or are we going to batch this PR with some others?

const variantClassName = styles[`badge-${variant}`]

return (
<Box {...rest} className={classNames(styles.badge, variantClassName)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Box actually implements classNames under the hood (ref), so this could be simplified:

Suggested change
<Box {...rest} className={classNames(styles.badge, variantClassName)}>
<Box {...rest} className={[styles.badge, variantClassName]}>

src/new-components/badge/badge.stories.mdx Show resolved Hide resolved
src/new-components/badge/badge.tsx Show resolved Hide resolved
src/new-components/badge/badge.stories.mdx Outdated Show resolved Hide resolved
Comment on lines 23 to 31
<Column width="content">
<Badge variant="neutral">Upgrade</Badge>
</Column>
<Column width="content">
<Badge variant="positive">Experiment</Badge>
</Column>
<Column width="content">
<Badge variant="color">Beta</Badge>
</Column>
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 I wonder if it would be better to use the variant as the label... I might be having a slow day, but it took me a second to figure out which variant is which 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I originally wanted to match the mocks, but yeah this would make it easier to understand.

@frankieyan
Copy link
Member Author

Thanks @pawelgrimm! I've addressed your feedback, please have another look 🙏

Will you be creating a new minor version or are we going to batch this PR with some others?

I usually add a changelog entry to next and only bump versions after changes are merged, in case other changes need to go in in the meantime 👍

Copy link
Contributor

@pawelgrimm pawelgrimm left a comment

Choose a reason for hiding this comment

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

🚀

@pawelgrimm
Copy link
Contributor

I usually add a changelog entry to next and only bump versions after changes are merged, in case other changes need to go in in the meantime 👍

I think that's ok, but I wonder if we can refine this process a bit more. After all, it's possible that after merging to master, but before creating the release, someone else merges to master. Then, if you were to push changes to create your release without realizing that additional merged happened, you would end up creating a release that includes changes you hadn't accounted for. Unlikely at the current pace of development, but not impossible 🤔

It would be very nice if we could include some identifier in a PR that specifies the type of version bump and a changelog entry and then have the CI take care of creating and releasing the new version based on the current state of main at the time of your merge 🤩

@frankieyan
Copy link
Member Author

@pawelgrimm Ernesto created an issue here to get the conversation started: #675

Hopefully, we can prioritize an internal DO to tackle this 👍

@frankieyan frankieyan enabled auto-merge (squash) November 1, 2022 06:03
@frankieyan frankieyan merged commit d12cea2 into main Nov 1, 2022
@frankieyan frankieyan deleted the frankie/badge-component branch November 1, 2022 06:08
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.

None yet

2 participants