-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Enhancement : Badge Component #66555
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
The |
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.
Thanks for taking the initiative here!
I know we're still discussing component specs in the original issue, so I'll leave the technical review for later and just leave administrative notes for now. The main thing is that this component needs to start as a private API. It needs to be a locked export, and include a 'status-private'
tag in the Storybook docs.
Hi @mirka 👋 Thanks for your input
Let me know your thoughts on it. Thanks |
Hey @Vrishabhsk what's the latest here? Has the component been updated to reflect the recent discussions in the issue? Let me know if I can help. |
Hi @jameskoster 👋 |
background-color: mix($white, $base-color, 90%); | ||
color: mix($black, $base-color, 50%); |
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.
@WordPress/gutenberg-design Have we considered what we want to happen when a Badge is placed on a dark background? (This can be a follow-up)
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.
What's the concern?
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.
Just thinking forward about theming. The default badge is subtle on a white background, but eye catching on a dark one. Color differences are also harder to perceive, I think, especially in isolation.
FWIW, the default badge looks like this when rendered in the current theming system:
(To be clear, these issues aren't blockers for this PR to land.)
{ context !== 'neutral' && ( | ||
<Icon | ||
icon={ contextBasedIcon() } | ||
size={ 20 } |
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.
There aren't many (any?) examples of icons rendering at 20px
in the software, but I don't mind it here. What do you think @jasmussen?
24px
looks a bit odd next to the smaller text:
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.
+1 on the 20px
rendering! That works well for the visual consistency.
Thank you @jameskoster for covering it and your work on these icons!
BTW In the initial designs I even had it set to 16px
.
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.
Hey, thanks for the ping, sorry for the belated response.
I have semi strong opinions on this, all related to the icons being carefully designed to be displayed at 24x24 precisely, so that the pixels render correctly. Even if they are SVGs meant to scale, the pixels won't alias perfectly at any resolution.
That's a long winded way of saying: if we are going to scale the icons down, we should do it according to some careful and deliberate math, so the pixels alias as well as possible. And we should do it very rarely: it should be an exception to the rule to use scaled icons, essentially, just like how we have heuristics for typography sizes. This badge can be such an exception, certainly.
So about that math: the icons are 24x24 with a 1.5px stroke width. At 20x20, the stroke width would be 1.25px. That would render blurry both at 1x screens, and at 2x screens (2.50px). If we scaled the icon to 16 (33% smaller icon), the stroke width would be exactly 1px, so it would be crisp at both 1x and 2x screens.
How would 16x16 look in that context, can you try? If the math checks out, it should look very crisp, even if smaller. And smaller in this case would be good insofar as it looks intentional: it's smaller enough to be significant.
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.
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 quite like that, because it makes the icon feel like it's part of the text, almost like an in-text glyph rather than an icon. But curious what others think.
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.
@jameskoster @jasmussen I completely agree with the concerns raised here. The 16px icon rendered differently in Figma for me, but I agree with @jameskoster that it looks a bit off in the screenshot above. At the same time, I agree with @jasmussen that size-wise, it blends nicely with the text, which was also my goal.
The issue, though, is that the icon wasn’t designed for this size—its stroke width looks a tiny bit too thin when rescaled, and it doesn’t fill the crop area effectively.
Would it make sense to add a set of icons to the library specifically for badges at this smaller size? These could have adjusted stroke widths and proportions to better suit their context.
BTW Google does it well :D
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.
Would it make sense to add a set of icons to the library specifically for badges at this smaller size? These could have adjusted stroke widths and proportions to better suit their context.
This is a bit of a large undertaking, and even now we are missing a build process (#38850) so this is likely not feasible in the foreseeable future. In that vein, I'd either keep the icons at 16px size, try 18px, just use 24x24, or omit the icon entirely.
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 vote we use the current icon at 16px. The resulting strokes are exactly 1px which I agree is a little thin, but should render sharply.
In the future it might be beneficial to redraw the icons to use stroke-width, then we can be a bit more precise/flexible. I discuss this a bit in #65786.
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.
The resulting strokes are exactly 1px which I agree is a little thin, but should render sharply.
Agreed. This is especially important for 1x resolution monitors, which is still the overwhelming majority of desktop monitors.
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.
In the future it might be beneficial to redraw the icons to use stroke-width, then we can be a bit more precise/flexible. I discuss this a bit in #65786.
This would also potentially open the door for some animation fun. Agreed this is a big undertaking.
# Conflicts: # packages/icons/CHANGELOG.md
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.
Thank you for the follow-ups @Vrishabhsk! I pushed some minor fixups surfaced in the failing CI checks, but otherwise I think we're pretty much ready to land this initial iteration.
Just waiting on some confirmation from the design team about icon file naming.
*/ | ||
import { SVG, Path } from '@wordpress/primitives'; | ||
|
||
const caution = ( |
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.
@WordPress/gutenberg-design I just want to make sure we measure twice and cut once here because publishing a new icon is kind of irreversible 😄
Would it be worth renaming these so it follows the convention of foo
+ fooFilled
like we do for the other pairs like this? For example, change the canonical name of warning
to cautionFilled
(maintaining back compat with an aliased export of course).
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.
Oh, yes, good suggestion. Just to clarify; that means any existing instances of warning
would map to cautionFilled
?
We should update the cautionFilled
icon so that the details match the new caution
icon too. Do you think it makes sense to do that in this PR, or separately?
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 means any existing instances of
warning
would map tocautionFilled
?
Correct 👍
Do you think it makes sense to do that in this PR, or separately?
Yes, let me pull the icon changes out of this PR so we can address it separately with proper care. I'll prepare a PR so you can push up changes to the SVG.
@Vrishabhsk Keeping this PR on hold for just a bit more. Thank you for your patience!
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.
@jameskoster Updates to the SVG can be pushed here: #67895
# Conflicts: # packages/icons/CHANGELOG.md # packages/icons/src/index.js
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.
Ok, I think the initial iteration is ready to be merged! Thank you everyone for the collaboration 🎉
* Create badge component * Imports and Manifest * Add test and capability for additional props using spread operator * Enhance componenet furthermore * Generate README via manifest & Add in ignore list * Lock Badge * Convert Storybook from CSF 2 to CSF 3 Format * Improve the component * New iteration * Add new icons: Error and Caution * Utilize new icons * Update icons * decrease icon size * Address feedback * Fix SVG formatting * Fix unit test * Remove unnecessary type (already included) * Update readme * Adjust icon keywords * Add changelog --------- Co-authored-by: Vrishabhsk <vrishabhsk@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: rogermattic <magdarogier@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org>
* Create badge component * Imports and Manifest * Add test and capability for additional props using spread operator * Enhance componenet furthermore * Generate README via manifest & Add in ignore list * Lock Badge * Convert Storybook from CSF 2 to CSF 3 Format * Improve the component * New iteration * Add new icons: Error and Caution * Utilize new icons * Update icons * decrease icon size * Address feedback * Fix SVG formatting * Fix unit test * Remove unnecessary type (already included) * Update readme * Adjust icon keywords * Add changelog --------- Co-authored-by: Vrishabhsk <vrishabhsk@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: rogermattic <magdarogier@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org>
What?
component
:Badge
Why?
reusable component
Example Use Cases
Data-Views
Diff
Post Card Panel
Diff
Screenshots