Skip to content

Remove margin from Badge component #241

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

Merged
merged 2 commits into from
Apr 15, 2020
Merged

Remove margin from Badge component #241

merged 2 commits into from
Apr 15, 2020

Conversation

vine77
Copy link
Contributor

@vine77 vine77 commented Apr 13, 2020

The left/right outer margins for Badge are inconsistent with our other components that don't have any outer margin. We otherwise require the consumer to set margins between components. This change will require any consumers relying on this behavior to add the margins to their consuming app.

This issue was noticed by me in nebula-ui where a Badge got overly indented due to not removing these margins and by @sprokusk in relay-website who overrode this margin to 0.

The only exception to react-components specifying margins is in _button.scss#L356-L358 but that is strictly adding margins only between multiple Buttons components.

@vine77 vine77 requested a review from a team as a code owner April 13, 2020 22:33
@vine77
Copy link
Contributor Author

vine77 commented Apr 13, 2020

I would be fine doing something similar to Button if the intention was just to add margin between Badge components:

.rc-badge + .rc-badge {
  margin-left: $puppet-common-spacing-base;
}

Copy link
Contributor

@caseywilliams caseywilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may already override this on the Forge, but even if we don't I'm 👍 (I don't feel strongly either way about the + rule you mention)

@vine77
Copy link
Contributor Author

vine77 commented Apr 15, 2020

Just talked to Sig who said that the intention was to get spacing between badges and approved the adjacent sibling selector approach, so I just pushed a commit for that.

Thanks for approving Casey!

@vine77 vine77 merged commit b14f3bb into development Apr 15, 2020
@vine77 vine77 deleted the remove-badge-margin branch April 15, 2020 17:22
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.

2 participants