Uh oh!
There was an error while loading. Please reload this page.
Corrects styling on Buttons that only have a count and icon - #5448
Conversation
🦋 Changeset detectedLatest commit: dbfbadb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Uh oh! @mperrotti, the image you shared is missing helpful alt text. Check your pull request body. Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
size-limit report 📦
|
langermank
left a comment
There was a problem hiding this comment.
Discussed on Slack and summarizing my thoughts here!
I would suggest putting this logic in Button to avoid adding a wrapper span inside IconButton. In order to utilize Button's layout wrapper .ButtonContent we're adding it to IconButton which feels like its adding unnecessary complexity given that this already exists in Button 😄
I would suggest...
- Add a data-attribute to
ButtonBasewhencountis truedata-has-countfor example - Add a CSS selector
&:where([data-has-count])and check if.Labelis not there (not has?) - Remove padding if has count is true,
.Labelis not present. You could also check that a leading visual is present but I'm not sure that's necessary
If this doesn't work out I would feel better about that span being conditional in IconButton!
Uh oh!
There was an error while loading. Please reload this page.
| export const TrailingCounterWithNoText = () => { | ||
| return <Button leadingVisual={CommentIcon} count={3} /> | ||
| } | ||
| export const TrailingCounterWithNoText = () => <Button leadingVisual={CommentIcon} count={3} /> |
There was a problem hiding this comment.
Should we add an aria-label or a Tooltip to this example?
There was a problem hiding this comment.
Hmmm I'm torn on this. I'd prefer not to encourage tooltips, but it's common to use them for icon-only buttons.
Maybe add a second example in the same story?
langermank
commented
Dec 18, 2024
You'll need to remove the snapshots that were added, and if you want add a new VRT test for the feature story 👍 |
Closeshttps://github.com/github/primer/issues/4501
Changelog
New
<Button>where there is only an icon and a counter label to closer align with<IconButton>sizingChanged
Nothing
Removed
Nothing
Rollout strategy
Testing & Reviewing
Confirm that this change doesn't cause any regressions with existing IconButton behavior.
Merge checklist