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

Remove redundant brackets #27768

Merged

Conversation

MartijnCuppens
Copy link
Member

There are a lot of redundant brackets in the scss, this PR cleans this up a bit. I still left the brackets on some places if it increased the readability of the code like here:

border-width: ($popover-arrow-width / 2) 0 ($popover-arrow-width / 2) $popover-arrow-height;

@XhmikosR
Copy link
Member

XhmikosR commented Dec 5, 2018

I don't think there's a rule for this, right? In stylelint-scss.

scss/_functions.scss Outdated Show resolved Hide resolved
@MartijnCuppens
Copy link
Member Author

@XhmikosR, no there isn't at the moment. My gut feeling tells me that'll also be a hard one to implement.

@XhmikosR
Copy link
Member

XhmikosR commented Dec 5, 2018

LGTM AFAIK, but let's wait to hear from @mdo too

@mdo
Copy link
Member

mdo commented Dec 7, 2018

I'm fine to do either way here—having the parentheses helped me with readability and consistency across the project. Obviously a stylistic thing, so if folks don't want them, I'm fine with it. Would be also nice to have a linter for it though as @XhmikosR was suggesting.

@XhmikosR
Copy link
Member

XhmikosR commented Dec 8, 2018

My main concern here is that if we can't automate the check, it's moot.

And having the extra brackets is a personal choice. I'm not against it, except for the cases it makes the expression more clear, but unless we can automate it, we'll just have to keep an eye for it manually, which is something not reliable.

@MartijnCuppens
Copy link
Member Author

We're actually quite inconsistent with this, for example these calculations don't use brackets:

$h1-font-size: $font-size-base * 2.5 !default;
$h2-font-size: $font-size-base * 2 !default;
$h3-font-size: $font-size-base * 1.75 !default;
$h4-font-size: $font-size-base * 1.5 !default;
$h5-font-size: $font-size-base * 1.25 !default;
$h6-font-size: $font-size-base !default;

$input-height-border: $input-border-width * 2 !default;

padding-right: $btn-padding-x * .75;
padding-left: $btn-padding-x * .75;

My main concern here is that if we can't automate the check, it's moot.

There are other things we always need to check ourself like the leading and trailing whitelines of titles in markdown files. I don't really like the idea that you can use whatever style you like here. Linting it would definitely be the best option, but I'm afraid this would be a plugin that is difficult to create, because you can have calc functions, scss variables, multiple value per property,...

scss/_variables.scss Outdated Show resolved Hide resolved
@XhmikosR
Copy link
Member

Let's go with it, just a 2-3 cases I think the brackets make it clearer.

@MartijnCuppens MartijnCuppens force-pushed the v4-dev-martijncuppens-remove-redundant-brackets branch from a7ea0a5 to 7ff2375 Compare December 14, 2018 16:35
@MartijnCuppens MartijnCuppens merged commit 502b6c8 into v4-dev Dec 14, 2018
@MartijnCuppens MartijnCuppens deleted the v4-dev-martijncuppens-remove-redundant-brackets branch December 14, 2018 16:54
@mdo mdo mentioned this pull request Dec 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants