Configure postcss formatting after stylelint and stylelint-config-standard update

Added just prettier command
And runned it.
Got lot of formatting errors

About
https://www.npmjs.com/package/stylelint-prettier

It seems not easy to configure fast. Also version 4 should be used.

Actionable title

This is reported as a regression caused by updating Stylelint to 15, which was committed on July 17, 2023.

I spot-checked the first three files in the merge request, and they are change code that was in place prior to the Stylelint changes on July 17, 2023, so it looks like this MR is making changes and enforcing rules that were enforced by any Stylelint config in core. Some of these changes look nice, but they'd need to be proposed as changes to the configured standard, as they do not appear to be regressions.

Also worth noting, in Stylelint's "Significant changes" page for version 15 it points out

The deprecated rules will still work in this release (with a deprecation warning). In preparation for the next major release, when we'll remove the rules from Stylelint, we suggest:

. So while it is true these rules are deprecated and we should prepare for their removal, they should currently work as they did in earlier versions.

Thank you for review. I marked it as major because any bad formatting can get into core without pipeline report

Prettier is definitely more strict and opinionated than Stylelint. As #6 says the bad formatting was previously there in many cases and Stylelint did not consider it a problem. While we now use Prettier for formatting PostCSS output, I don't think we ever applied the Prettier rules to unprocessed module CSS or the source *.pcss.css files.

If we *are* going to do this we should change the Prettier command in package.json:

    "prettier": "prettier --write \"./**/*.js\"",

to

    "prettier": "prettier --write \"./**/*.css\" \"./**/*.js\"",

Preparation for Stylelint 16 is happening in 📌 Update stylelint(-config-standard) to latest versions (major bump) Active

There is also ongoing work to update the CSS coding standards and perhaps an option is to adopt Prettier for opinionated formatting?

Added proof of regressions MR

It shows that stylelint 15 don't check formatting anymore

And the funniest thing here that 2 of 9 formatting errors committed by me :)

And one more merge requests about any formatting errors can get into core

Updated first MR 3409048-postcss-file-formatting with https://github.com/prettier/stylelint-prettier

Now it reports formatting errors

I think we may move with `3409048-postcss-file-formatting` MR with lint fixed

It is in fact does same work and postcss compilation prettier:

Rebased it. Mostly sure that results will be same.

On proof-of-failures MR we got new `max-line-length`, it was recently removed:
https://git.drupalcode.org/project/drupal/-/commit/fafc0e753deb811afb113...

but other reports are actual

Reverted max-line-length in proof of failures. To display only actual errors.

Guess this needs frontend manager to pick which proposal

I rebased and pushed formatted css in new commit:

So config fix:
https://git.drupalcode.org/project/drupal/-/merge_requests/5827/diffs?co...

Formatter fixes:
https://git.drupalcode.org/project/drupal/-/merge_requests/5827/diffs?co...

They look fair. Please review.

Overall I think we should just do this as it is only whitespace and quoting fixes, and makes all our CSS more consistent - if our compiled CSS uses prettier then why shouldn't our base CSS use it too?

However I think we can improve some comment placements, so added some feedback.

Thank you for review.
This was result of automated fix.
OK. Let's add some manual formatting

Fixed feedbacks. Please review!

Spotted a few things in the MR I'd like updated, but hopefully nothing major. Overall this looks like a good addition by adding CSS support to the prettier functionality we're already accustomed to with our JS files, and the formatting changes are not too jarring.

Spellcheck issue happened because of https://www.drupal.org/project/drupal/issues/3401988 🐛 Spell-checking job fails with "Argument list too long" when too many files are changed Active

Now all fine, review still needed.

Blocker resolved, follow up to reuse .prettierrc

finnsky → changed the visibility of the branch 11.x to hidden.

finnsky → changed the visibility of the branch 11.x to hidden.

Added feedback to the MR. It is all trivial other than one thing, and even the non-trivial one shouldn't be too demanding. Overall looks good and looking forward to rtbcing.

I’ve fixed common .prettierrc for both compile and lint.

But about comments in media queries i think they are out of scope requests in current bugfix ticket.

I’ve added approach when we will not have blank lines and which will work fine with postcss-custom-media

https://git.drupalcode.org/project/drupal/-/merge_requests/5827/diffs?co...

if it is acceptable i can manage it globally. otherwise move it to follow up. thank you

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

This bot too fast ;)

It still needs review. One branch here for failures example.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Thank you! This robot was stronger than me ;)

Appears to be some open threads if they can closed or marked complete.

Can the follow up mentioned in #40 be added also

MR appears to need a rebases.

Moving to NW for those 3 items.

Been using that config standard on most of my projects and will say like it a lot :)

re #52

#40 fixed already.

finnsky → changed the visibility of the branch 3409048-postcss-file-formatting to hidden.

I think we need to leave the current MR. In which all the main problems are solved and autofix is launched.

Positioning comments in each individual weird CSS is clearly outside of this task.

So we should add this as is to https://git.drupalcode.org/project/drupal/-/merge_requests/7024/diffs

And if necessary, add subsequent tasks.

Rebased after yarn update. Please review.

It's a +1 for me but will leave in review for @bnjmnm as the frontend framework manager.

Re the media query comments #56

Positioning comments in each individual weird CSS is clearly outside of this task.

I think it belongs in the scope of this issue as the formatting changes implemented by this issue changes the position of these comments. I think having them on the same line or above a media query is similarly useful, having them underneath the MQ declaration makes them less so. If there are any specific ones that are particularly difficult to reposition, removing is also OK.

So for > me < to provide FEFM signoff, I want to see those media query PX comments positioned above. That said, if another FEFM is fine with it I will not protest that decision. I'm not comfortable approving as-is, but if one of my peers is that's fine by me.

@bnjmnm what about follow up issue?

Because here we are fixing a bug.
Right now, unformatted css is getting into the core.

And with the current rate of reviews, this is becoming almost impossible to fix. Because this commit is completely rebase unfriendly

I've rebased. I think we need to fix this formatting bug.

59 problems (59 errors, 0 warnings)

From new navigation module. Before i used force push in one commit. Here i gonna add it as example.

about #60 given that this is an automated MR, I'd be reluctant to introduce manual changes on top of it. I did it for the once() replace patch and don't recommend it because of the difficulty to reroll. IMO The comments were placed awkwardly to begin with, so it make sense that an automated fix would place them not where we want.

Given that this is an automated patch, I'm fine with the comments in this issue but a follow-up needs to be created to move them either above or before the opening bracket.

Since we'll use prettier for css, we might want to enable css file rewrites in the prettier command of our package.json. Ok with a follow-up for that since we'll need to exclude a bunch of files/directories from this auto-formatting and it'd make all this longer.

I'm committing a bunch of issues with css changes to make the RTBC queue go down, so we'll have to reroll that just before commit.

I think we should address #10 from @longwave in a follow-up as the command yarn prettier currently makes changes even if we are on HEAD. Opened 📌 Add CSS to prettier command and fix core/misc/jquery.form.js to be ignored Active

Committed 1fbf9f5 and pushed to 11.x. Thanks!

Needs backport to 10.4.x and probably 10.3.x too.

alexpott → changed the visibility of the branch 3409048-config-and-autofix to hidden.

alexpott → changed the visibility of the branch proof-of-failures to hidden.

alexpott → changed the visibility of the branch success-on-anything to hidden.

Created an MR against 10.4.x and ran the fixer...

Didn't know we had a 10.4 branch going now. But rerun for it seems fine

btw is a follow up still needed mentioned in #30 or framework manager approval since it was committed to 11.x already?

@smustgrave - @nod_, a frontend framework manager rtbc'd this issue in #65

Committed and pushed 5c9cc42046 to 10.4.x and 306ab17ec7 to 10.3.x. Thanks!

Backported to 10.3.x as major bug fix.

longwave → closed merge request !7872