Problem/Motivation

Package                   Current Wanted  Latest   
postcss                   8.4.19  8.4.21  8.4.21   
postcss-import            15.0.1  15.1.0  15.1.0   
postcss-preset-env        7.8.3   7.8.3   8.3.0

Steps to reproduce

Proposed resolution

$ yarn add -D postcss-preset-env
$ yarn upgrade postcss postcss-import

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 yarn_build_css_no_update.patch316.66 KBspokje
#10 3351619-9.patch86.62 KBgauravvvv
#8 3351619-8.patch392.03 KBgauravvvv
#2 3351619-2.patch85.14 KBlongwave

Issue fork drupal-3351619

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Active » Needs review
StatusFileSize
new 3351619-2.patch85.14 KB

postcss-preset-env now adds -webkit-text-decoration everywhere text-decoration is used due to a Safari issue: https://github.com/postcss/autoprefixer/issues/1473

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

No failures.

Not sure the level of concern -webkit-text-decoration being added everywhere. Assuming none just mentioning.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

There is no concern from me, just a note to reviewers in case they are surprised by the diff like I was.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Reviewed & tested by the community » Needs review

Actually Olivero's variables.css has some odd looking changes, would like a second opinion on those from a frontend developer.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel as a volunteer commented

Something's looking weird here. I'm seeing lots of additions of -webkit-text-decoration, which makes absolutely no sense since that's a very old and well supported property.

In general this need to be done, as it's the correct resolution for #3313146: PostCSS Logical not transpiling flow relative properties (e.g. float: inline-start) which are not supported by Chrome and Safari, where PostCSS Preset ENV now includes https://www.npmjs.com/package/@csstools/postcss-logical-float-and-clear (see https://github.com/csstools/postcss-plugins/issues/632)

I'm going to pull it down and take a closer look.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel as a volunteer commented
Status: Needs review » Needs work

The patch no longer applies to 10.1.x

I ran the following commands:

$ yarn add -D postcss-preset-env
$ yarn upgrade postcss postcss-import

Then I ran yarn build:css.

The CSS that was generated for me was different than the patch above:

In addition, the new version is moving around some style blocks, which could potentially have regressions (although I doubt it actually will).

Setting this to NW based on all of this. I wish I had more time to work on this, but I don't at the moment.

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
StatusFileSize
new 3351619-8.patch392.03 KB

Patch #2, no longer applies to 10.1.x. I have re-rolled the patch. please review

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work

Umm that patch is almost 4 times the size of patch 2.

Also mentioned in the comment before there were some issues. Did you run the same steps mentioned in #7?

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
StatusFileSize
new 3351619-9.patch86.62 KB

Above patch has some errors (size is too high), I have rebased the branch and provided the latest patch.

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented

Also mentioned in the comment before there were some issues. Did you run the same steps mentioned in #7?

Yes I ran the same steps, it might be because of my branch was not up to date. I have provided the patch after rebasing the branch.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented
StatusFileSize
new yarn_build_css_no_update.patch316.66 KB

So, "interestingly" when I do a yarn install followed by a yarn build:css , so _without_ updating anything, I get the changes in the attached patch, which seem just to have (very) wrong indentations.

EDIT: This looks like somehow #3314523: PostCSS results in CSS that does not comply with our coding standards got reverted, as already said by @mherchell in #7, but it appears unrelated to updating the packages in this issue, since it's already happening without any updates.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented

If I revert #3344087: Update Stylelint and Prettier for Drupal 10.1 and use Prettier for formatting PostCSS and run yarn build:css I get the same CSS as in the HEAD.
So it looks like that issue broke something

Also, we don't seem to be running yarn build:css whenever core/package.json and/or core/package.lock changes in our commit checks, which would have caught the above, me thinks.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

I've reverted #3344087: Update Stylelint and Prettier for Drupal 10.1 and use Prettier for formatting PostCSS. Stylelint 15 has deprecated indentation and other opinionated style rules, and they have been removed from stylelint-config-standard 30. This is why the reformatting no longer works. We will have to migrate to Prettier or similar to continue reformatting our CSS.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented

Let's wait until #3344087: Update Stylelint and Prettier for Drupal 10.1 and use Prettier for formatting PostCSS is committed before proceeding here, to prevent endless rerolling because of conflicts in yarn.lock.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented

#3344087: Update Stylelint and Prettier for Drupal 10.1 and use Prettier for formatting PostCSS landed.

Gauravvvv opened merge request !3903

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel as a volunteer commented
Status: Needs review » Needs work

This is still adding the unnecessary -webkit-text-decoration properties to the CSS. Haven't looked at why, but this is not needed. Webkit has support the unprefixed version since 2014.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review

@mherchel this is to work around a bug in Safari, see https://github.com/postcss/autoprefixer/issues/1473

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel as a volunteer commented

OMG Safari 🤦‍♂️

Will look at this closely today (if no one beats me to it)

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel as a volunteer commented
Status: Needs review » Reviewed & tested by the community

This looks good! I pulled down the patch, and verified the CSS (paying special attention to the changes in Olivero's variables.css).

I also verified no changes to the CSS when I do a yarn build:css, and I also verified the yarn watch:css functionality.

I also verified that the new syntax of CSS nesting works (removes @nest, & not always needed, etc). At some point we'll convert to that.

Note that I have a couple questions upstream that don't affect us quite yet, and I opened up an issue at https://github.com/csstools/postcss-plugins/issues/958

  • catch committed 0bfcd0a6 on 10.1.x 
    Issue #3351619 by Gauravvvv, longwave, Spokje, mherchel, smustgrave:...

  • catch committed 3a34afe9 on 11.x 
    Issue #3351619 by Gauravvvv, longwave, Spokje, mherchel, smustgrave:...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 10.1.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.