Skip to content

fix 23034: added Variable for Navbar padding #23109

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

fix 23034: added Variable for Navbar padding #23109

wants to merge 5 commits into from

Conversation

@chiraggmodi
Copy link
Contributor

@chiraggmodi chiraggmodi commented Jul 17, 2017 •
edited by bardiharborow
Loading

I have added $nav-link-padding in variables.scss. Fixes #23034.

wojtask9
wojtask9 reviewed Jul 19, 2017
View reviewed changes
scss/_list-group.scss Outdated

.list-group-flush {
.list-group-item {
border-top: 0;
Copy link
Contributor

@wojtask9 wojtask9 Jul 19, 2017

Choose a reason for hiding this comment

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

this change is unrelated.

Copy link
Contributor Author

@chiraggmodi chiraggmodi Jul 19, 2017

Choose a reason for hiding this comment

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

yes sorry for that, actually this is for another PR

scss/_variables.scss Outdated

// Navs

$nav-link-padding: .5rem !default;
Copy link
Contributor

@wojtask9 wojtask9 Jul 19, 2017

Choose a reason for hiding this comment

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

I would call this $navbar-link-padding and put with others navbar-* variables.
Also consider if split between x/y axis wouldn't be better

Copy link
Contributor Author

@chiraggmodi chiraggmodi Jul 19, 2017

Choose a reason for hiding this comment

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

yes that would be better will update this changes are PR again.

Copy link
Contributor Author

@chiraggmodi chiraggmodi Jul 20, 2017

Choose a reason for hiding this comment

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

I have made your requested changes, but for navbar link variable i have splited in left/right instead of x/y as this are for left and right side not left/right and top/bottom.

@chiraggmodi
Copy link
Contributor Author

chiraggmodi commented Jul 20, 2017

because if some folks wants to give different value then they can overwrite those variable.

@gijsbotje
Copy link
Contributor

gijsbotje commented Jul 20, 2017

I get that but then all the values with -x and -y would have to be changed to
I would suggest to leave this to the programmer to overwrite this in custom scss and just stick with -x

@chiraggmodi
Copy link
Contributor Author

chiraggmodi commented Jul 20, 2017

have change $navbar-link-padding variable to $navbar-link-padding-x

@wojtask9 wojtask9 mentioned this pull request Sep 19, 2017
Closed
@andresgalante andresgalante mentioned this pull request Sep 21, 2017
Closed
@mdo
Copy link
Member

mdo commented Jan 3, 2018

This was superseded by #25117

@mdo mdo closed this Jan 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@wojtask9 wojtask9 wojtask9 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

css v4

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@chiraggmodi @gijsbotje @mdo @wojtask9 @Johann-S @chiragatlas