Problem/Motivation
When we started the development for Claro, we kept our post-processed and the source CSS assets in different subdirectories.
Right before Claro was committed to core, #3060153: Use PostCSS in core, initially only for Claro landed as well, and it uses a different approach for managing these assets by keeping the source and the post-processed CSS files in the same folder.
On my opinion, there is no reason anymore to keep the CSS files inside the src
subfolder.
Proposed resolution
- Move every CSS assets from
./css/scr/
to./css/
. - Refactor the
url()
references.
Remaining tasks
Patch, review.
User interface changes
Expected: nothing at all! (No broken images or pseudo content!)
API changes
Nothing.
Data model changes
Nothing.
Release notes snippet
8.8.0-alpha1 introduced the experimental Claro administrative theme, which uses PostCSS to compile versions of its CSS compatible with all of Drupal 8.8's supported browsers. In that release, the unprocessed CSS was in a src
subdirectory, but this subdirectory was in the same directory as the compiled CSS. 8.8.0-rc1 simplifies this directory structure so that the sources and compiled CSS are in the same directory.<
Comment | File | Size | Author |
---|---|---|---|
#31 | interdiff-3088805-27-31.txt | 19.08 KB | huzooka |
#31 | claro-simplify_css_dir-3088805-31.patch | 79.39 KB | huzooka |
#27 | interdiff-3088805-23-27.txt | 14.32 KB | huzooka |
#27 | claro-simplify_css_dir-3088805-27.patch | 79.83 KB | huzooka |
#23 | interdiff-3088805-20-23.txt | 7.76 KB | huzooka |
Comments
Comment #2
huzookaComment #3
huzookaI also removed some redundant images (except of the icons on the status report page).
Comment #4
huzookaNote: these RTL styles are still missing from Seven theme.
Comment #5
huzookaComment #6
huzookaComment #8
martin107 CreditAttribution: martin107 as a volunteer commentedA little fix.
Comment #9
fhaeberle+1
Comment #10
huzookaJust asked @lauriii why we test our
elements.css
specifically.The answer is:
> It was the first asset listed in global-styling library.
I'd add this comment as well.
Comment #11
huzookaComment #12
huzookaComment #13
lauriiiI'm not sure we actually need this inline documentation. Updating the method documentation to be more specific on what we're testing should be sufficient.
Comment #14
huzookaComment #15
huzookaComment #16
huzookaComment #17
huzookaComment #19
huzookaComment #20
huzookaComment #21
lauriiiWhat are these additions? Shouldn't we be using icons from core for these?
Comment #22
huzookaRe #21:
These are only available in Seven theme.
Comment #23
huzookaComment #24
xjm@lauriii and I discussed separating the SVG improvements from simply moving the files around since these two changes require different review contexts.
We also discussed that this issue is probably OK to backport to 8.8.x during the beta phase since Claro is internal code; however, it's probably worth finishing before 8.8.0-rc1 if we're going to make the change.
Comment #25
huzookaI definitely think that this is finished.
Comment #26
huzookaFrom @lauriii on slack:
Comment #27
huzookaComment #28
huzookaFollow-up added: #3092984: Clean up Claro's unused images
Comment #29
lauriiiThank you, this is already much easier to review! Let's move these changes to #3092984: Clean up Claro's unused images as well.
Comment #30
huzookaComment #31
huzookaBroken image references:
core/themes/claro/css/components/jquery.ui/theme.pcss.css
: the background image of.ui-progressbar .ui-progressbar-value
was broken (and I cannot find the originally referenced asset)core/themes/claro/css/components/menus-and-lists.pcss.css
: the background of the collapsed/expanded menu list items were also broken and the collapsed state misses the RTL styleComment #32
lauriiiI reviewed that we're only making changes to the directory structure by using
git diff --color-words=.
. I also grepped allurl(../
instances and manually confirmed they are pointing to a valid path. Everything looks good now! 👏Comment #33
alexpottCommitted f1e9b8a and pushed to 9.0.x and cherry-picked it to 8.9.x and 8.8.x as @xjm's +1 on backport. Thanks!
Comment #37
xjmThis should go in the RC release notes since it's a semi-disruptive change from beta1. (It doesn't need to go in the 8.8.0 notes since the theme didn't exist in 8.7.)
Comment #38
xjm