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.

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

Switch to stylelint. #23572

Merged
merged 2 commits into from
Oct 3, 2017
Merged

Switch to stylelint. #23572

merged 2 commits into from
Oct 3, 2017

Conversation

XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Aug 20, 2017 •
edited
Loading

Fixes #23383

Note to all: feel free to push to this branch, and merge from v4-dev, but please do not rebase until we are ready to merge to v4-dev.

TODO:

  • Make sure tests are green
  • Check for properties order
  • Tighten rules
  • Make sure Hound works

@wojtask9
Copy link
Contributor

wojtask9 commented Aug 21, 2017 •
edited by XhmikosR
Loading

Probably you also want to use stylelint-order plugin.

@XhmikosR
Copy link
Member Author

Yup, it's on my todo, but I thought I'd let the other team members chime in before I make a lot more changes myself :)

@andresgalante
Copy link
Collaborator

It'd be great if we make style lint format the code or use another formater.
Fornater + lint = happiness

Love this change 👍🏻

@XhmikosR XhmikosR force-pushed the v4-dev-xmr-stylelint branch 2 times, most recently from cec53e5 to 6a90f48 Compare September 15, 2017 14:27
@XhmikosR XhmikosR mentioned this pull request Sep 15, 2017
Merged
@XhmikosR XhmikosR force-pushed the v4-dev-xmr-stylelint branch 2 times, most recently from 39fb7f0 to 9126bae Compare September 17, 2017 05:22
@XhmikosR XhmikosR force-pushed the v4-dev-xmr-stylelint branch 6 times, most recently from 3d0ab49 to e5a0ea7 Compare September 30, 2017 07:14
@twbs twbs deleted a comment from houndci-bot Sep 30, 2017
@twbs twbs deleted a comment from houndci-bot Sep 30, 2017
@twbs twbs deleted a comment from houndci-bot Sep 30, 2017
@twbs twbs deleted a comment from houndci-bot Sep 30, 2017
@twbs twbs deleted a comment from houndci-bot Sep 30, 2017
@twbs twbs deleted a comment from houndci-bot Sep 30, 2017
@twbs twbs deleted a comment from houndci-bot Sep 30, 2017
@twbs twbs deleted a comment from houndci-bot Sep 30, 2017
@twbs twbs deleted a comment from houndci-bot Sep 30, 2017
@twbs twbs deleted a comment from houndci-bot Sep 30, 2017
@twbs twbs deleted a comment from houndci-bot Sep 30, 2017
@twbs twbs deleted a comment from houndci-bot Sep 30, 2017
@twbs twbs deleted a comment from houndci-bot Oct 1, 2017
@twbs twbs deleted a comment from houndci-bot Oct 1, 2017
@twbs twbs deleted a comment from houndci-bot Oct 1, 2017
@twbs twbs deleted a comment from houndci-bot Oct 1, 2017
@twbs twbs deleted a comment from houndci-bot Oct 1, 2017
@twbs twbs deleted a comment from houndci-bot Oct 1, 2017
mdo
mdo reviewed Oct 1, 2017
View reviewed changes
package.json
@@ -117,9 +122,7 @@
"build",
"js/.eslintrc.json",
"js/**/*.js",
".scss-lint.yml",
"scss/**/*.scss",
"LICENSE"
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the license stay?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's default included in npm.

@XhmikosR XhmikosR force-pushed the v4-dev-xmr-stylelint branch 2 times, most recently from 7d96457 to 1a610d8 Compare October 1, 2017 20:01
@twbs twbs deleted a comment from houndci-bot Oct 1, 2017
@twbs twbs deleted a comment from houndci-bot Oct 1, 2017
@twbs twbs deleted a comment from houndci-bot Oct 1, 2017
@twbs twbs deleted a comment from houndci-bot Oct 1, 2017
@twbs twbs deleted a comment from houndci-bot Oct 1, 2017
houndci-bot
houndci-bot reviewed Oct 2, 2017
View reviewed changes
scss/mixins/_reset-text.scss
@mixin reset-text {
font-family: $font-family-base;
// We deliberately do NOT reset font-size or word-wrap.
font-style: normal;
font-weight: $font-weight-normal;
line-height: $line-height-base;
text-align: left; // Fallback for where `start` is not supported
text-align: start;
text-align: start; // stylelint-disable-line declaration-block-no-duplicate-properties
Copy link

Choose a reason for hiding this comment

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

Property text-align already defined on line 7

scss/_navbar.scss
.navbar-collapse {
display: flex !important;
display: flex !important; // stylelint-disable-line declaration-no-important
Copy link

Choose a reason for hiding this comment

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

!important should not be used

scss/_modal.scss
@@ -89,7 +89,8 @@

.close {
padding: $modal-header-padding;
margin: (-$modal-header-padding) (-$modal-header-padding) (-$modal-header-padding) auto; // auto on the left force icon to the right even when there is no .modal-title
// auto on the left force icon to the right even when there is no .modal-title
margin: (-$modal-header-padding) (-$modal-header-padding) (-$modal-header-padding) auto;
Copy link

Choose a reason for hiding this comment

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

Shorthands of length 4 are not allowed. Value was -$modal-header-padding -$modal-header-padding -$modal-header-padding auto

assets/scss/_anchor.scss
.anchorjs-link {
font-weight: normal;
font-weight: 400;
Copy link

Choose a reason for hiding this comment

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

Properties should be ordered color, font-weight, transition

assets/scss/_algolia.scss
@@ -25,7 +27,7 @@
display: none !important;
}

[class^=ds-dataset-] {
[class^="ds-dataset-"] {
Copy link

Choose a reason for hiding this comment

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

Selector should have depth of applicability no greater than 2, but was 3

houndci-bot
houndci-bot reviewed Oct 2, 2017
View reviewed changes
scss/mixins/_reset-text.scss
@mixin reset-text {
font-family: $font-family-base;
// We deliberately do NOT reset font-size or word-wrap.
font-style: normal;
font-weight: $font-weight-normal;
line-height: $line-height-base;
text-align: left; // Fallback for where `start` is not supported
text-align: start;
text-align: start; // stylelint-disable-line declaration-block-no-duplicate-properties
Copy link

Choose a reason for hiding this comment

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

Property text-align already defined on line 7

scss/_navbar.scss
.navbar-collapse {
display: flex !important;
display: flex !important; // stylelint-disable-line declaration-no-important
Copy link

Choose a reason for hiding this comment

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

!important should not be used

scss/_modal.scss
@@ -89,7 +89,8 @@

.close {
padding: $modal-header-padding;
margin: (-$modal-header-padding) (-$modal-header-padding) (-$modal-header-padding) auto; // auto on the left force icon to the right even when there is no .modal-title
// auto on the left force icon to the right even when there is no .modal-title
margin: (-$modal-header-padding) (-$modal-header-padding) (-$modal-header-padding) auto;
Copy link

Choose a reason for hiding this comment

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

Shorthands of length 4 are not allowed. Value was -$modal-header-padding -$modal-header-padding -$modal-header-padding auto

assets/scss/_anchor.scss
.anchorjs-link {
font-weight: normal;
font-weight: 400;
Copy link

Choose a reason for hiding this comment

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

Properties should be ordered color, font-weight, transition

assets/scss/_algolia.scss
@@ -25,7 +27,7 @@
display: none !important;
}

[class^=ds-dataset-] {
[class^="ds-dataset-"] {
Copy link

Choose a reason for hiding this comment

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

Selector should have depth of applicability no greater than 2, but was 3

@XhmikosR XhmikosR mentioned this pull request Oct 2, 2017
Closed
@XhmikosR
Copy link
Member Author

XhmikosR commented Oct 2, 2017

@mdo: is there anything else holding us for this? Otherwise pull the trigger 🔫

@mdo mdo merged commit 7b766e1 into v4-dev Oct 3, 2017
@mdo mdo deleted the v4-dev-xmr-stylelint branch October 3, 2017 03:35
@mdo mdo mentioned this pull request Oct 3, 2017
Closed
@bardiharborow bardiharborow mentioned this pull request Oct 4, 2017
Merged
@zalog zalog mentioned this pull request Jan 12, 2018
Closed
@rodrigobdz rodrigobdz mentioned this pull request Feb 5, 2019
Merged
@Omrisnyk Omrisnyk mentioned this pull request Jul 26, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdo mdo mdo left review comments

@houndci-bot houndci-bot houndci-bot left review comments

@bardiharborow bardiharborow Awaiting requested review from bardiharborow

@Johann-S Johann-S Awaiting requested review from Johann-S

Assignees
No one assigned
Labels
build v4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@XhmikosR @wojtask9 @andresgalante @bardiharborow @wolfy1339 @mdo @houndci-bot