-
Notifications
You must be signed in to change notification settings - Fork 536
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
Allow CircleBadge size to be customized #213
Conversation
src/CircleBadge.js
Outdated
@@ -29,4 +29,4 @@ CircleBadge.propTypes = { | |||
src: PropTypes.string | |||
} | |||
|
|||
export default withSystemProps(CircleBadge, COMMON) | |||
export default withSystemProps(CircleBadge, [...COMMON, 'width', 'height']) |
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.
You could use size
here instead of width
and height
; that'll set both of them uniformly.
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.
Ooh didn't know that!
Thanks for getting this rolling @emplums. ❤️ FWIW, I thought I'd mention that we're also using the current |
…nto update-circle-badge
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.
Nice! I like this with size
instead of having to mirror width
and height
.
…nto update-circle-badge
One note from component API review yesterday: The way this is written leaves string values for the We might even consider having named sizes ("small", "medium", "large", etc.) defined in the theme. |
…nto update-circle-badge
src/CircleBadge.js
Outdated
return ( | ||
<Tag className={classes} {...rest}> | ||
{mappedChildren} | ||
</Tag> | ||
) | ||
} | ||
|
||
const CircleBadge = styled(Badge)` |
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.
You should also be able to do this with:
const CircleBadge = withSystemProps(Badge, [...COMMON, sizeStyles])
This PR adds the
width
andheight
system props to CircleBadge in order to allow users to specify a custom size for their CircleBadge. I did this in response to @jasonlong reaching out because Launch uses 48px CircleBadges and the smallest CircleBadge in Primer is 56px. I could see the argument either way for not allowing a smaller badge but I felt this was a case were customization of size felt alright.I also considered adding a
CircleBadge--extra-small
to Primer, but felt there might be more cases where a CircleBadge would need a specific size so opted for full customization instead.Don't feel particularly strongly about this decision though so happy to be swayed another direction :) cc @broccolini @shawnbot @jonrohan
Closes #211
If development process was changed:
Description of changes:
Merge Checklist: