Problem/Motivation

Our current CSS aggregation strategy normally uses 4 different aggregate files (though theoretically, it could use up to 6 separate files.) That's too many files. We can get that down to just 2 files (one for CSS needed on every page and one for CSS needed for a specific set of pages.)

The proposed aggregation strategy is discussed in detail over at http://drupal.org/node/1887922#aggregate

Proposed resolution

This is the first step in a proposed solution outlined at #1921610: [Meta] Architect our CSS. In short, we are changing our 3 aggregate group constants to a single group constant and adding 5 new "weight"-based CSS constants.

This patch is just the first step. It renames the current CSS_* constants to be CSS_AGGREGATE_* and adds 5 new CSS_* constants to be used for weights. It also updates the docs for drupal_add_css().

Remaining tasks

All the other steps outlined at #1921610: [Meta] Architect our CSS.

User interface changes

none

API changes

All the CSS_* constants are being renamed to CSS_AGGREGATE_*.

CommentFileSizeAuthor
#9 css-order-before.jpg170.86 KBcarwin
#9 cache_cleared.jpg17.36 KBcarwin
#9 css-order-after.jpg156.87 KBcarwin
#8 before_patch.jpeg146 KBykhadilkar
#8 after_patch.jpeg160.95 KBykhadilkar
#1 1924368-1-css-constants.patch11.21 KBJohnAlbin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Status: Active » Needs review
FileSize
1924368-1-css-constants.patch11.21 KB

Here's the patch.

Shyamala’s picture

Shyamala CreditAttribution: Shyamala commented
Issue tags: +mobile, +d8mux, +d8mux-css-cleanup

Adding tags

LittleCoding’s picture

LittleCoding
He/Him
Location Hamilton, ON
CreditAttribution: LittleCoding commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Code is well formated.

This could use more test coverage to do with the new approach to css aggregation.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs work » Needs review

@LittleCoding and myself reviewed this patch together during Sprint Weekend. I suggested tagging for tests but after looking at the patch again I'm reconsidering that. Back to CNR for some more reviews :)

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented

Actually, this patch doesn't need to add tests. Because the system already has sufficient tests for drupal_add_css(); it tests aggregating and ordering. This patch just changes the CSS constants used. We don't need to test PHP's ability to set constants. :-)

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented

This patch doesn't change the behavior of the Drupal in any way. Maybe that's making it tricky to review.

So maybe these review instructions would help:

  1. Turn off CSS aggregation.
  2. Visit the /admin page using the Seven and observe the ordering of the following CSS files: system.base.css, system.admin.css, system.theme.css, Seven's style.css, and Seven's ie.css
  3. Apply the patch
  4. Flush the cache
  5. Visit the /admin page again using the Seven and ensure the ordering of the files listed in Step 2 is the same.
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue tags: -Needs tests

Agreed, my apologies. Updating tags.

ykhadilkar’s picture

ykhadilkar CreditAttribution: ykhadilkar commented
FileSize
after_patch.jpeg160.95 KB
before_patch.jpeg146 KB

Followed steps in #6. CSS ordering did not change. Used firebug to verify loaded CSS. Not sure if this is correct way of identifying CSS load ordering. Please see attached screenshots.

carwin’s picture

carwin CreditAttribution: carwin commented
Status: Needs review » Reviewed & tested by the community
FileSize
css-order-after.jpg156.87 KB
cache_cleared.jpg17.36 KB
css-order-before.jpg170.86 KB

I can confirm the ordering doesn't change, RTBC time. Awww yeah!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: Rename old CSS constants to CSS_AGGREGATE_* and add new CSS weight constants » Change notice: Rename old CSS constants to CSS_AGGREGATE_* and add new CSS weight constants
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Well, that's definitely much nicer! Thanks for the manual testing as well!

Committed and pushed to 8.x.

This'll need a change notice.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Title: Change notice: Rename old CSS constants to CSS_AGGREGATE_* and add new CSS weight constants » Rename old CSS constants to CSS_AGGREGATE_* and add new CSS weight constants
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Added change notice: http://drupal.org/node/1980980

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