Add and configure stylelint-order

+1 on automating coding standards checks to reduce manual work developers have to do.

I'd like to express my opinions without wearing any hats since I don't think there's a right or wrong way to order selectors. I don't think ordering selectors alphabetically makes it much better than having properties in random order, whereas grouped by type actually makes it easier to read the CSS. It is also not as commonly used as grouping by type at least according to this poll.

Hi Folks,

Great to see this progressing. I'm adding two patches for consideration here:

1. Adds support for object-fit and object-position
2. Adds support for object-fit and object-position and also @include breakpoint. Not sure if we want the breakpoint support in a core linter, but many people using sass use the breakpoint package for easier to write/read media queries, and like the $media property, we need these after our other declarations for mobile-first theming.

#9 applied cleanly, passes linting.

Further to #8 and @alexpott's comment about breakpoint support, I'm wondering if there is a place here to add some additional rules for properties that aren't currently included in core, most notably grid, grid-area, grid-template, etc? These aren't used atm, but that could conceivably change considering the current state of support - caniuse grid support.

I guess this comes down to, should new properties be added when they come up as a failsafe, or should it just be focussed on what's currently in core?

If so, some additional properties (not including prefixes) could include:

all All @ MDN

grid
grid-area
grid-template
grid-template-areas
grid-template-rows
grid-template-columns
grid-row
grid-row-start
grid-row-end
grid-column
grid-column-start
grid-column-end
grid-auto-rows
grid-auto-columns
grid-auto-flow
grid-gap
grid-row-gap
grid-column-gap

perspective perspective @MDN perspective @ caniuse
perspective-origin

There may be more, but that's what I've noticed with this pass.

Happy to make a patch with these if there's a place for the additional properties.

@occupant,

Thanks.

I would be 100% for adding grid support. I think anything that helps developers to use the tools from core (such as this linting set-up) without having to extend or fork it is a great help in getting everyone (automatically) following the coding standards.

If people have to extend or fork, we run a real risk of people just adopting some other standards in their own work because the things they want are in them, and then finding core/contrib harder to work with when they have to change tack.

So, for me, given the prevalence of CSS Grid, I think that should certainly be a part of this file.

@markconroy

I'd love to see this in there too. Here's a patch with the properties added. I had to change the order of align-self, as it's used in grid as well. I've also attempted adding the -ms prefixes as per how they're added by autoprefixer.

Can you add an interdiff between this patch and the last one please, so we can see exactly what has changed?

Whoops, I should have done that. I've rerolled it again and created an interdiff between this and patch #3.

Edit: hmm, not sure I did that right. are the /core/package.json and /core/yarn.lock changes supposed to be in there?

@alexpott they’re used by css grid and flexbox - css grid guide @css tricks, so my thinking was it doesn’t make sense to come before the grid property. I see I did miss align-content and justify-content however, which are both grouped above with flexbox atm. I would expect that these should appear lower as well.

That said, the exact order could very well be different than what I’ve proposed.

Let's put align-self back where it was, as it is a property of flexbox. https://developer.mozilla.org/en-US/docs/Web/CSS/align-self

And let's move justify-self to either directly above or directly below the flex properties as it is an alignment property that is not strictly part of flex or grid, can be used with position and display https://developer.mozilla.org/en-US/docs/Web/CSS/justify-self

Adding new patch for what I propsed above in #17

@markconroy I think it's actually a shared property, as per your link:

The align-self CSS property aligns flex items of the current flex line overriding the align-items value. If any of the item's cross-axis margin is set to auto, then align-self is ignored. In Grid layout align-self aligns the item inside the grid area.

So I think, as a shared property, I'd argue these should come either before or after both flex and grid properties to normalize the expected order.

I propose the following ordering of these shared properties:

place-content //shorthand for align-content and justify-content
place-items //shorthand for align-items and justify-items
align-content
align-items
align-self
justify-content
justify-items
justify-self

order

Thoughts?

Just looking through the flex properties, and it seems that the syntax is outdated.

Deprecated
flex-order // replaced by order https://developer.mozilla.org/en-US/docs/Web/CSS/flex-order
flex-pack // replaced by justify-content https://developer.mozilla.org/en-US/docs/Web/CSS/flex-pack
flex-align // replaced by justify-items https://developer.mozilla.org/en-US/docs/Web/CSS/flex-align
align // not foundhttps://developer.mozilla.org/en-US/docs/Web/CSS/align

Also, the flex-flow shorthand property is missing. With this and #19 in mind, I'd propose updating the flex properties to:

flex
flex-flow // shorthand for flex-direction and flex-wrap, so should precede both
flex-direction
flex-wrap
flex-basis
flex-grow
flex-shrink

New patch with following changes:

Added "order/order" config to match CSSComb so that variables (and one-line sass @at-rules) sit before declarations but at-rules with blocks (@media, @supports, sass @include breakpoint) sit after declarations and before rules.

Worth noting that "order/properties-order" ignores variables ($sass, @less, --custom-property).
This means I don't think it's possible to have certain at-rules like @keyframe name {} block in between transform and animation declarations like it is in CSSComb.

Removed "$charset", "$import", "$namespace", "$extend", "$variable", "$include" from beginning of "order/properties-order" and "$media", "$include breakpoint", "$supports", "$document", "$page" and "$viewport" from end of "order/properties-order" as they area now dealt with by the "order/order" rules before.

However left other "$declaration" for reference in case we want to look at ordering them in a specific place.

Based on comments in #19 and #20 I've updated the flex properties, and moved the flexbox & grid shared alignment properties to below both, that does seem to make sense.

Setting back to 'Needs review' to get the tests to run again. I don't think the failures are anything to do with the patch (which looks great).

I did a screenshare with @waako a few minutes ago and got him to show me this working. I'm _very_ impressed with it.

I'm happy with the ordering of items, the spacing between block sets, how custom variables (--black: #000;) are placed at the top of our rules, followed by any $dollar variables (for SASS, etc - $c-black: #000), then our general css (color: #000;), and then any media queries/supports/etc at the end.

Here's a screenshot of some made-up code before we run the stylelint:

And here's a screenshot of this mess all cleaned up:

I think this will make a great addition to Drupal core, and will mean sooo many developers across the world can use it as the base for their linting when developing Drupal websites, without having to fork it or extend it, given how it works for CSS/SCSS/etc.

Thanks @waako for this and @occupant for adding the grid stuff.

The tests are passing now. Looks like Drupal Infra had an issue.

I'm marking this as RTBC, it's a great improvement.

We might want to postpone running the linters until later in Beta/RC phase.

Small change in "order/order" placing at-rule blocks after rules.

This doesn't impact on Drupal core CSS, but will help with preprocessors and nested rules being overriden inside @supports or @include breakpoint statements.
Without this change the @supports block would be placed before the nested rules it aims to override and be overriden themselves, defeating the purpose of the @supports altogether.

Updated 8.7.x branch locally, hence the large interdiff.

@alexpott, can we mark this RTBC again? You happen with this new patch - bigger, better, newer!!!

Hi @alexpott

#3038562: Update stylelint to 9.10.1 is RTBC now as well.

Thank you @alexpott for fixing those issues in #27.
I definitely got myself mixed up with my local git branches considering it was such a small change, compared to correct patch & interdiff I did in #21. I live and learn.

I am having the errors while testing the patch to the 8.8.x branch

error1:

Checking patch core/modules/quickedit/css/quickedit.theme.css...
error: while searching for:
 * In-place editors that don't use a popup.
 */
.quickedit-validation-errors .messages.error {
  box-shadow: 0 0 1px 1px red, 0 0 3px 3px rgba(153, 153, 153, 0.5);
  background-color: white;
}

/**
 * Styling specific to the 'form' in-place editor.
 */
.quickedit-form {
  box-shadow: 0 0 30px 4px #4f4f4f;
  background-color: white;
}

/**
 * Toolbars.
 */
.quickedit-toolbar-container {
  font-family: "Source Sans Pro", "Lucida Grande", sans-serif;
  padding-bottom: 7px;
  padding-top: 7px;
  -webkit-transition: all 1s;
  transition: all 1s;
}
.quickedit-toolbar-container > .quickedit-toolbar-content {
  background-image: -webkit-linear-gradient(top, #fff, #e4e4e4);
  background-image: linear-gradient(to bottom, #fff, #e4e4e4);
  box-sizing: border-box;
  color: black;
  padding: 0.1667em;
  position: relative;
  -webkit-user-select: none;
  -moz-user-select: none;
  -ms-user-select: none;
  user-select: none;
  z-index: 2;
}
.quickedit-toolbar-container > .quickedit-toolbar-pointer {
  background-color: #e4e4e4;
  bottom: 2px;
  box-shadow: 0 0 0 1px #818181, 0 0 0 4px rgba(150, 150, 150, 0.5);
  display: block;
  height: 16px;
  left: 18px; /* LTR */
  position: absolute;
  -webkit-transform: rotate(45deg);
  -ms-transform: rotate(45deg);
  transform: rotate(45deg);
  width: 16px;
  z-index: 1;
}
[dir="rtl"] .quickedit-toolbar-container > .quickedit-toolbar-pointer {
  left: auto;
  right: 18px;
}
.quickedit-toolbar-container.quickedit-toolbar-pointer-top > .quickedit-toolbar-pointer {
  bottom: auto;
  top: 2px;
}
.quickedit-toolbar-container > .quickedit-toolbar-lining {
  bottom: 7px;
  box-shadow: 0 0 0 1px #818181, 0 3px 0 1px rgba(150, 150, 150, 0.5);
  display: block;
  left: 0;
  position: absolute;
  right: 0;
  top: 7px;
  z-index: 0;
}

.quickedit-toolbar-label {
  font-style: italic;
  overflow: hidden;
  padding: 0.333em 0.4em;
  text-overflow: ellipsis;
  white-space: nowrap;
}
.quickedit-toolbar-label .field:after {
  content: " → "; /* LTR */

error: patch failed: core/modules/quickedit/css/quickedit.theme.css:83

error2:

Checking patch core/themes/stable/css/quickedit/quickedit.theme.css...
error: while searching for:
 * In-place editors that don't use a popup.
 */
.quickedit-validation-errors .messages.error {
  box-shadow: 0 0 1px 1px red, 0 0 3px 3px rgba(153, 153, 153, 0.5);
  background-color: white;
}

...
error: patch failed: core/themes/stable/css/quickedit/quickedit.theme.css:83

We may need to re-patch the css files for quickedit styles

https://github.com/drupal/drupal/blob/8.8.x/core/modules/quickedit/css/q...

/**
 * In-place editors that don't use a popup.
 */
.quickedit-validation-errors .messages.error {
  box-shadow: 0 0 1px 1px red, 0 0 3px 3px rgba(153, 153, 153, 0.5);
  background-color: white;
}

The deadline for this for 8.7.x has passed, so untagging 'til next time. Thanks!

@xjm If this is not the right time to commit this kind of disruptive patch when is?

@xjm,

I'm adding back in the 8.7.0 release notes tag.

We deliberately didn't commit this early in the 8.7.x branch phase because it would be very disruptive to many CSS-based patches. We were leaving it until feature-freeze/beta/rc so it has as little disruption as possible, and then people working on CSS-based patches can use this when we transition to the 8.8.x branch.

@alexpott and @lauriii discussed this with me on Slack a few months back.

@Joseph Zhao - CSS files are broken. That's why we need this patch, so the coding standards will be automated. quickedit.css et all will be fixed automatically once this patch is committed. It won't apply directly against the quickedit module per se, it'll be run and that will update the quickedit.css file for us. Then when people do their next git pull, they'll have CSS files that follow our coding standards.

That seems very strange. This has been RTBC pretty much for a month, but we held back on it then for no good reason it seems.

I hope the same doesn't happen in 6 months when we are trying to get it into 8.8.x

@xjm I don't mean to go against the decision. I'm just looking for clarity because I know these can be disruptive so timing is key so I just want to get it correct. Also for contrib projects I'm maintaining there is similar disruptive patches that would be nice to have a message for when the patch can make it in.

@markconroy

thanks for the clarification.

Commit to 8.8.x then backported to 8.7.x makes sense

Is the proposal to commit #49 to 8.8.x, and after that, we would create another patch where the library addition has been removed, but it still contains the same CSS changes and commit that to 8.7.x? I like the plan except I would prefer if we would create both patches at the same time, and only commit this to 8.8.x if we also commit the CSS changes to 8.7.x. If we end up in a situation where we committed these CSS changes only to 8.8.x, backporting CSS changes to 8.7.x would in most cases require a separate patch.

Looks good for me. I talked with @catch on Slack who is another release manager and he didn't express any particular concerns on this approach. So committed the change with the library addition to Drupal 8.8.x with the CSS changes and only the CSS changes to 8.7.x

@lauriii

Thanks for committing this. It'll be a great help to people to follow the coding standards.

I'm a little confused at the specific reason why we wouldn't include it in 8.7. if we are including the CSS that it generates. I don't see how anything would break and it will make following coding standards to be a lot easier for an extra six months.

Where's the separate issue for the library addition?

@lauriii clarified that he manually fixed 8.7.x without backporting the library rather than committing a separate patch, so the commit message there misled me.

WRT not backporting the library after beta, this is not an arbitrary release manager decision. It is documented core policy: library additions are only allowed in minor versions, and as such need to be completed before beta, as per:

The release of the first beta is a firm deadline for all feature and API additions. Even if an issue is pending in the RTBC queue when the commit freeze for the beta begins, it will be committed to the next minor release only.

I'd prefer not to make exceptions based on what kind of library it is because we have to explain or justify them; the whole point of the alpha and beta phases is that alpha is "with exceptions" and beta is "no exceptions".

In an ideal world, the library would have been added, but not used, before the beta deadline, and the cleanup would have been scheduled during beta.

Thanks!