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

Allow CircleBadge size to be customized #213

Merged
merged 17 commits into from
Aug 17, 2018

Conversation

emplums
Copy link

@emplums emplums commented Aug 14, 2018

This PR adds the width and height 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:

  • Updated README?

Merge Checklist:

@@ -29,4 +29,4 @@ CircleBadge.propTypes = {
src: PropTypes.string
}

export default withSystemProps(CircleBadge, COMMON)
export default withSystemProps(CircleBadge, [...COMMON, 'width', 'height'])
Copy link
Contributor

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.

Copy link
Author

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!

@shawnbot shawnbot changed the title Allow CircleBadge size to be cuztomized Allow CircleBadge size to be customized Aug 14, 2018
@jasonlong
Copy link

Thanks for getting this rolling @emplums. ❤️

FWIW, I thought I'd mention that we're also using the current small size in Launch. This new smaller option is for a different context where small is still a bit too big.

Copy link
Contributor

@shawnbot shawnbot left a 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.

@shawnbot
Copy link
Contributor

One note from component API review yesterday:

The way this is written leaves string values for the size prop in CSS. E.g. size="small" results in emotion-generated CSS width: small; height: small, which the Primer CSS styles for our size variations take precedence over. When we're rewriting this (and other components that have named "shortcuts" for numeric sizes) we'll need to implement our own style function that maps props.size to a value in an object before it gets to the size function from styled-system.

We might even consider having named sizes ("small", "medium", "large", etc.) defined in the theme.

@shawnbot shawnbot mentioned this pull request Aug 16, 2018
8 tasks
return (
<Tag className={classes} {...rest}>
{mappedChildren}
</Tag>
)
}

const CircleBadge = styled(Badge)`
Copy link
Contributor

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])

@emplums emplums merged commit dd98e34 into release-2.0.0-beta Aug 17, 2018
@emplums emplums deleted the update-circle-badge branch August 17, 2018 17:52
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