-
Notifications
You must be signed in to change notification settings - Fork 24
fix: Change Badge component to bold + set explicit line height #755
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
Conversation
378d96c to
d5f9d2d
Compare
Errr not true (see this build -- you can see that there's no side by side comparison of all the badges). Looking into it now 👀 Edit: I think the inside the main storybook demo prevents the tests from working correctly 🤔 |
src/badge/badge.module.css
Outdated
| padding: var(--reactist-badge-paddingY) var(--reactist-badge-paddingX); | ||
| color: var(--reactist-badge-tint); | ||
| background-color: var(--reactist-badge-fill); | ||
| line-height: calc(var(--reactist-badge-font-size) + 2 * var(--reactist-badge-paddingY)); |
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 think it would be a bit simpler if instead of applying a vertical padding, we just apply a line height and that's it. That alone would enforce the height and the vertical spacing around the text, even if it's not padding, right?
With that in mind, I would also hard-code the line-height to be font-size + 2px, instead of doing all this calculation. Something along these lines:
.badge {
padding: 0 var(--reactist-spacing-xsmall);
line-height: calc(var(--reactist-badge-font-size) + 2);
}I also feel that offering badge-specific CSS variables for the padding of the badge may be unnecessary. I'd rather add that API later if needed, than committing to it today. Any new CSS variables we add becomes public Reactist API that we later need to support or deprecate if we want to remove it or change it.
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.
That makes a lot of sense, thank you, I will test this now.
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.
|
@engfragui upon inspecting the Chromatic snapshots: I find it odd that in there the vertical padding looks almost non-existent. Could it be that 1px is too little? |
@gnapse Errr the Figma mocks say the padding should be 3px 😅 I think it still looked good because the background was anyway expanding vertically to fill the entire vertical height, so we never noticed this. I will change to 3px. |




Short description
This PR tweaks our (existing) Badge component:

Although this can be easily set in the client, I figured we would want to enforce this in Reactist _once and for all_.I'm also taking this chance to fix a couple of typos in the Banner stories (25 instead of 24 px -- I was probably drunk, who knows 🤷♀️).
PR Checklist
npm run validateand made sure no errors / warnings were shownCHANGELOG.mdpackage.jsonandpackage-lock.json(npm --no-git-tag-version version <major|minor|patch>) refnpm run build-all)Versioning
I think this is a minor fix, so I'm just bumping the patch.