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

Refactor a few selectors. #24404

Merged
merged 12 commits into from
Oct 22, 2017
Merged

Conversation

XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Oct 17, 2017

  1. move a couple of them before the more specific ones.
  2. change nesting to be under the same parent selector
  3. use the ampersand more

1. move a couple of them before the more specific ones.
2. change nesting to be under the same parent selector
3. use the ampersand more
h4,
h5,
h6 {
margin: 0;
Copy link
Member

Choose a reason for hiding this comment

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

I think we can change this to margin-top: 0; and margin-bottom: 0;. Can't imagine we're adding horizontal margin to any headings.

&.fade .modal-dialog {
@include transition($modal-transition);
transform: translate(0, -25%);
.modal-open & {
Copy link
Member

Choose a reason for hiding this comment

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

In general I'm not a fan of this type of nesting—usually ends up more difficult to parse in more complicated rulesets.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we need to keep this; it doesn't complicate things a lot and along with the complexity rule we should be fine. I plan to enable that later too.

I removed the & & instances.

scss/_code.scss Outdated
@@ -31,7 +31,7 @@ kbd {
@include border-radius($border-radius-sm);
@include box-shadow($kbd-box-shadow);

kbd {
& & {
Copy link
Member

Choose a reason for hiding this comment

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

This is weird at first, but probably fine.

Copy link
Collaborator

@andresgalante andresgalante left a comment

Choose a reason for hiding this comment

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

@XhmikosR sorry that I couldn't help much with this one. Thanks for doing it. You made a linter happy :)

@XhmikosR
Copy link
Member Author

@andresgalante: this isn't quite ready since I haven't enabled the rule yet. Still waiting for your patch so that we enable the rule. :)

@andresgalante
Copy link
Collaborator

:p lol I thought it was! ok I'll continue with it

@XhmikosR XhmikosR changed the title Enable stylelint's no-descending-specificity rule Refactor a few selectors. Oct 22, 2017
@XhmikosR
Copy link
Member Author

@andresgalante: I'm gonna merge this as is, and we should tackle the 2 remaining stylelint rules later.

@XhmikosR XhmikosR merged commit 52d99a8 into v4-dev Oct 22, 2017
@XhmikosR XhmikosR deleted the v4-dev-xmr-no-descending-specificity branch October 22, 2017 21:17
@mdo mdo mentioned this pull request Oct 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants