Skip to content

Add margin variable for paragraphs#23140

Merged
mdo merged 5 commits intotwbs:v4-devfrom
prateekgoel:variable-for-paragraph-margin
Aug 11, 2017
Merged

Add margin variable for paragraphs#23140
mdo merged 5 commits intotwbs:v4-devfrom
prateekgoel:variable-for-paragraph-margin

Conversation

@prateekgoel
Copy link
Contributor

@prateekgoel prateekgoel commented Jul 22, 2017

I added a section for paragraphs in _variables.scss, used that variable to change hard coded margin value for p element.

Fixes #23031

mdo
mdo requested changes Jul 25, 2017
View reviewed changes
Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

Paragraphs don't have horizontal margin on them (by default or from Bootstrap), so there's no need to reset them if there is nothing there to begin with.

Please restore the margin-top: 0; and use a variable for the margin-bottom.

Also, no need to shorten the word to four letters—use $paragraph-margin-bottom or something like that maybe?

@prateekgoel
Copy link
Contributor Author

prateekgoel commented Jul 25, 2017 •
edited
Loading

Ok. I'll do these changes. Thanks.
I think $paragraph-margin-bottom should be fine.
@mdo Shall I add a variable for margin-top also instead of restoring margin-top: 0;?

@prateekgoel
Copy link
Contributor Author

prateekgoel commented Jul 27, 2017

@mdo For now, I have restored margin-top: 0;
Let me know if anything else need to be done.

@prateekgoel
Copy link
Contributor Author

prateekgoel commented Aug 6, 2017

@mdo Any update on this?

mdo
mdo requested changes Aug 7, 2017
View reviewed changes
Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

This'll go in for Beta 2 at the earliest.

scss/_variables.scss Outdated
$breadcrumb-padding-x: 1rem !default;
$breadcrumb-item-padding: .5rem !default;

$breadcrumb-margin: 0 0 1rem !default;
Copy link
Member

@mdo mdo Aug 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This shouldn't be here

Copy link
Contributor Author

@prateekgoel prateekgoel Aug 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@mdo Should I add breadcumb-margin too? I mean this PR is for paragraph-margin. For breadcrumb there's a different PR #23138 .

Copy link
Member

@mdo mdo Aug 7, 2017

Choose a reason for hiding this comment

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

Crap, I typo-ed—I meant shouldn't be here. It's in the wrong PR, so you can remove it.

Copy link
Contributor Author

@prateekgoel prateekgoel Aug 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@mdo Changes done.

@mdo mdo merged commit 7c452ac into twbs:v4-dev Aug 11, 2017
@mdo mdo mentioned this pull request Aug 11, 2017
Closed
@prateekgoel prateekgoel deleted the variable-for-paragraph-margin branch August 13, 2017 18:09
@andresgalante andresgalante mentioned this pull request Aug 14, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mdo mdo mdo requested changes

Assignees

No one assigned

Labels

css v4

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@prateekgoel @mdo @Johann-S