- Issue created by @finnsky
- Status changed to Needs work
9 months ago 2:04pm 15 December 2023 - 🇷🇸Serbia finnsky
Added just prettier command
And runned it.
Got lot of formatting errorsAbout
https://www.npmjs.com/package/stylelint-prettierIt seems not easy to configure fast. Also version 4 should be used.
- 🇺🇸United States bnjmnm Ann Arbor, MI
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.
- 🇷🇸Serbia finnsky
Thank you for review. I marked it as major because any bad formatting can get into core without pipeline report
- 🇬🇧United Kingdom longwave UK
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.
- 🇬🇧United Kingdom longwave UK
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\"",
- 🇬🇧United Kingdom longwave UK
Preparation for Stylelint 16 is happening in 📌 Update stylelint(-config-standard) to latest versions (major bump) Active
- 🇬🇧United Kingdom longwave UK
There is also ongoing work to update the CSS coding standards and perhaps an option is to adopt Prettier for opinionated formatting?
- 🇷🇸Serbia finnsky
Added proof of regressions MR
It shows that stylelint 15 don't check formatting anymore
- 🇷🇸Serbia finnsky
And the funniest thing here that 2 of 9 formatting errors committed by me :)
- Issue was unassigned.
- 🇷🇸Serbia finnsky
And one more merge requests about any formatting errors can get into core
- Status changed to Needs review
9 months ago 7:00am 16 December 2023 - 🇷🇸Serbia finnsky
Updated first MR 3409048-postcss-file-formatting with https://github.com/prettier/stylelint-prettier
Now it reports formatting errors
- 🇷🇸Serbia finnsky
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:
- 🇷🇸Serbia finnsky
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
- 🇷🇸Serbia finnsky
Reverted max-line-length in proof of failures. To display only actual errors.
- 🇺🇸United States smustgrave
Guess this needs frontend manager to pick which proposal
- 🇷🇸Serbia finnsky
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.
- Status changed to Needs work
8 months ago 11:19am 2 January 2024 - 🇬🇧United Kingdom longwave UK
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.
- 🇷🇸Serbia finnsky
Thank you for review.
This was result of automated fix.
OK. Let's add some manual formatting - Status changed to Needs review
8 months ago 8:10am 3 January 2024 - Status changed to Needs work
8 months ago 3:55pm 9 January 2024 - 🇺🇸United States bnjmnm Ann Arbor, MI
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.
- Status changed to Needs review
8 months ago 4:39pm 9 January 2024 - 🇷🇸Serbia finnsky
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.
- Status changed to Needs work
7 months ago 5:27pm 14 February 2024 - 🇺🇸United States bnjmnm Ann Arbor, MI
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.
- Status changed to Needs review
7 months ago 1:12pm 15 February 2024 - 🇷🇸Serbia finnsky
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
- Status changed to Needs work
7 months ago 10:45am 22 February 2024 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.
- Status changed to Needs review
7 months ago 10:51am 22 February 2024 - 🇷🇸Serbia finnsky
This bot too fast ;)
It still needs review. One branch here for failures example.
- Status changed to Needs work
7 months ago 12:59pm 22 February 2024 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.
- Status changed to Needs review
7 months ago 1:21pm 22 February 2024 - Status changed to Needs work
7 months ago 2:11pm 22 February 2024 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.
- Status changed to Needs review
7 months ago 4:21pm 22 February 2024 - Status changed to Needs work
6 months ago 5:27pm 13 March 2024 - 🇺🇸United States smustgrave
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 :)
- Status changed to Needs review
6 months ago 7:27pm 13 March 2024 - 🇷🇸Serbia finnsky
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.
- 🇺🇸United States smustgrave
It's a +1 for me but will leave in review for @bnjmnm as the frontend framework manager.
- 🇺🇸United States bnjmnm Ann Arbor, MI
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.
- 🇷🇸Serbia finnsky
@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
- 🇷🇸Serbia finnsky
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.
- Status changed to RTBC
4 months ago 5:19pm 28 April 2024 - 🇫🇷France nod_ Lille
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.
- 🇫🇷France nod_ Lille
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.
- First commit to issue fork.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
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 - Status changed to Downport
4 months ago 10:30am 1 May 2024 -
alexpott →
committed 1fbf9f5d on 11.x
Issue #3409048 by finnsky, alexpott, longwave, bnjmnm, andypost, nod_:...
-
alexpott →
committed 1fbf9f5d on 11.x
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
alexpott → changed the visibility of the branch 3409048-config-and-autofix to hidden.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
alexpott → changed the visibility of the branch proof-of-failures to hidden.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
alexpott → changed the visibility of the branch success-on-anything to hidden.
- Status changed to Needs review
4 months ago 4:46pm 1 May 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
Created an MR against 10.4.x and ran the fixer...
- Status changed to RTBC
4 months ago 9:12pm 1 May 2024 - 🇺🇸United States smustgrave
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?
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
@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.
-
alexpott →
committed 306ab17e on 10.3.x
Issue #3409048 by finnsky, alexpott, longwave, bnjmnm, andypost, nod_:...
-
alexpott →
committed 306ab17e on 10.3.x
- Status changed to Fixed
4 months ago 12:14am 2 May 2024 -
alexpott →
committed 5c9cc420 on 10.4.x
Issue #3409048 by finnsky, alexpott, longwave, bnjmnm, andypost, nod_:...
-
alexpott →
committed 5c9cc420 on 10.4.x