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

Badge component - Add font weight declaration #426

Merged
merged 2 commits into from
Jul 7, 2022

Conversation

didoo
Copy link
Contributor

@didoo didoo commented Jul 6, 2022

📌 Summary

Following this comment by @heatherlarsen #425 (comment) I've realized we never explicitly set the font-weight property for the text in the Badge component. This has two problems: 1) it inherits from the container, as Heather noticed, and 2) it was using the wrong weight, it should be "medium".

🛠️ Detailed description

In this PR I have:

  • added font-weight “medium” to the text in the Badge component

👀 How to review

👉 Review commit-by-commit or by files changed

Reviewer's checklist:

  • +1 Percy if applicable
  • Confirm that PR has a changelog update via Changesets if needed

💬 Please consider using conventional comments when reviewing this PR.

@vercel
Copy link

vercel bot commented Jul 6, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
hds-components ✅ Ready (Inspect) Visit Preview Jul 7, 2022 at 9:32AM (UTC)
hds-flight-website ✅ Ready (Inspect) Visit Preview Jul 7, 2022 at 9:32AM (UTC)

@changeset-bot
Copy link

changeset-bot bot commented Jul 6, 2022

🦋 Changeset detected

Latest commit: ff236b2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@hashicorp/design-system-components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

Makes sense! I'm not sure about the value of redefining this property for the required badge in this case.

@didoo
Copy link
Contributor Author

didoo commented Jul 6, 2022

Makes sense! I'm not sure about the value of redefining this property for the required badge in this case.

Yes, we shouldn't in that case.

Copy link
Contributor

@heatherlarsen heatherlarsen left a comment

Choose a reason for hiding this comment

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

This change also needs to be made for BadgeCount

@didoo didoo force-pushed the badge-add-font-weight-declaration branch from 68539bb to dbe79f7 Compare July 7, 2022 09:25
@vercel vercel bot temporarily deployed to Preview – hds-flight-website July 7, 2022 09:27 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components July 7, 2022 09:27 Inactive
@didoo didoo force-pushed the badge-add-font-weight-declaration branch from dbe79f7 to ff236b2 Compare July 7, 2022 09:30
@didoo
Copy link
Contributor Author

didoo commented Jul 7, 2022

This change also needs to be made for BadgeCount

@heatherlarsen 🤦‍♂️ I thought it was the same CSS for both. Now it's fixed.

@didoo didoo requested a review from heatherlarsen July 7, 2022 09:31
@vercel vercel bot temporarily deployed to Preview – hds-components July 7, 2022 09:31 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website July 7, 2022 09:32 Inactive
Copy link
Contributor

@heatherlarsen heatherlarsen left a comment

Choose a reason for hiding this comment

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

lgtm! thanks @didoo 🙂

@didoo didoo merged commit d8585ef into main Jul 7, 2022
@didoo didoo deleted the badge-add-font-weight-declaration branch July 7, 2022 15:17
@hashibot-hds hashibot-hds mentioned this pull request Jul 7, 2022
@hashibot-hds hashibot-hds mentioned this pull request Jul 18, 2022
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.

3 participants