Problem/Motivation

On #3060153: Use PostCSS in core, initially only for Claro PostCSS capability was added to Core.

The generated CSS files get this header in them:

/*
 * DO NOT EDIT THIS FILE.
 * See the following change record for more information,
 * https://www.drupal.org/node/2815083
 * @preserve
 */

The link in that header goes to this change record about JavaScript compiling:
https://www.drupal.org/node/2815083

It should instead go to the change record about PostCSS:
https://www.drupal.org/node/3084859

Proposed resolution

Novice task!

Generated CSS files should have the correct change record URL in their headers. Looking at the patch for the parent issue, it appears that these headers are generated from scripts, and the header source is included in two places:
core/scripts/css/compile.js
core/postcss.config.js
So, both of them need a 1-line change to fix the URL.

The other thing that needs to be done is to fix the headers in the existing CSS file output under the Claro theme (the only one in Core currently using PostCSS I think) . There are several ways this could be done:

a) The correct way is to follow the compiling procedure in the change record: https://www.drupal.org/node/3084859

b) The tedious and technically not correct way is to edit all of the .css files that do not have ".pcss" in their file names under core/themes/claro/css, and fix the URLs in their headers.

c) The less tedious and also technically incorrect way to do it would be to write a script that would replace the URLs.

I'm pretty sure the patch will end up the same any way it is done, but definitely (a) is the right way to do it if possible.

Remaining tasks

1. Make a patch as noted in Proposed resolution.
2. Review and commit the patch.

User interface changes

None.

API changes

None, but compiled CSS output from PostCSS input will be slightly different.

Data model changes

None.

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon created an issue. See original summary.

jhodgdon’s picture

Issue summary: View changes

Adding some notes to the summary.

Kristen Pol’s picture

Issue tags: +Global2020

Tagging this issue for new contributors to work on during the Friday DrupalCon Global sprint.

qjac’s picture

I'm working on it! DrupalCon 202 Contrib Day

laura.gates’s picture

I'll give this one a go for Global2020 :)

jhodgdon’s picture

Status: Active » Needs work

Thanks for the patch! A few thoughts:

a) When you upload a patch, set the issue status (under Issue metadata) to "Needs review". The needed tests will automatically launch. You don't have to add the tests manually, and we don't usually launch that many tests on one issue unless it is specifically needed. Setting to "Needs review" also alerts human reviewers that the patch is ready to be reviewed.

b) When I tried to apply the patch, I had to switch to the "core" directory to apply it. Always make patches using "git diff", which should make the patch relative to the Drupal root git directory instead of a subdirectory.

c) Your patch file didn't apply. If you click through to the test results, it says:

fatal: corrupt patch at line 494

I get the same result at the command line... Here is line 494:

diff --git a/themes/claro/css/components/system-admin--admin-list.css b/themes/claro/css/components/system-admin--admin-list.css

I am not sure what is going on... the file looks OK to me but there must be some weird character in the file. Maybe try making the patch again or re-uploading it?

laura.gates’s picture

Status: Needs work » Active
FileSize
33.59 KB

I rerolled the patch trying a strait git clone for the dev branch rather than doing composer create-project drupal/recommended-project my_site_name_dir

laura.gates’s picture

Status: Active » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the new patch! It seems to be correct for the CSS files, even though if you click through to the coding standards messages, you find that it generates 145 new coding standards messages. I have no idea why that would be.

That aside, the patch does not have the fixes for
core/scripts/css/compile.js
core/postcss.config.js
as mentioned in the issue summary. It only contains the fixes for the Claro output files.

laura.gates’s picture

Status: Needs work » Needs review
Kristen Pol’s picture

@laura.gates Thanks for the patches. It's helpful to have an interdiff included when patches are changed so we can see the changes easier: https://www.drupal.org/documentation/git/interdiff

laura.gates’s picture

@Kristen,

I think the issues I was having with the other two patches was from not using command line to save the patch.

jhodgdon’s picture

We're not worried about why the patches weren't correct... you are learning! :) Still, an interdiff file is still very helpful for reviewers, especially when the new patch is presumably fairly similar to the previous one with just a few small changes.

Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community

Marking RTBC since:

1) Patch applies cleanly.

2) Tests pass.

3) Patch changes 2815083 to 3084859 as described in the issue summary.

4) After applying the patch and searching for 2815083, the files found are all JavaScript files which is correct.

5) Searched for 3084859 and the files found are all CSS files in Claro plus core/scripts/css/compile.js and core/postcss.config.js which looks correct.

6) Searched for Claro CSS files that do NOT have 3084859 and they are either the pcss.css files or the following CSS files that don't have PCSS equivalents:

./css/classy/components/file.css
./css/classy/components/book-navigation.css
./css/classy/components/ui-dialog.css
./css/classy/components/item-list.css
./css/classy/components/field.css
./css/classy/components/tablesort.css
./css/classy/components/forum.css
./css/classy/components/inline-form.css
./css/classy/components/link.css
./css/classy/components/textarea.css
./css/classy/components/links.css
./css/classy/components/exposed-filters.css
./css/classy/components/indented.css
./css/classy/components/search-results.css
./css/classy/components/node.css
./css/classy/components/menu.css
./css/classy/components/icons.css
./css/classy/components/media-embed-error.css
./css/classy/components/container-inline.css
./css/classy/components/more-link.css
alexpott’s picture

Version: 9.1.x-dev » 9.0.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: +Documentation

Committed and pushed 1ba1cd8b78 to 9.1.x and d052178664 to 9.0.x. Thanks!

Backported because this is a docs fix

  • alexpott committed 1ba1cd8 on 9.1.x
    Issue #3155159 by laura.gates, jhodgdon, Kristen Pol: Fix notice at top...

  • alexpott committed d052178 on 9.0.x
    Issue #3155159 by laura.gates, jhodgdon, Kristen Pol: Fix notice at top...

Status: Fixed » Closed (fixed)

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

Chi’s picture

Give that's a bug why wasn't it backported to 8.9.x?