Problem/Motivation

Now that #3565258: Support library-specific aggregates and #1945262: Introduce "before" and "after" for conditional ordering in library definitions have landed we should have more opportunities to improve aggregate hit rates.

Since Olivero and Claro are on their way out of core and default_admin's libraries are still in flux, concentrating on CSS shipped with core modules.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3615642

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:

Comments

catch created an issue. See original summary.

bluegeek9 made their first commit to this issue’s fork.

bluegeek9 changed the visibility of the branch main to hidden.

catch’s picture

Status: Active » Needs review
needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

catch’s picture

Status: Needs work » Needs review

Rebased.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

This should work as not sure how often the navigation css changes so this shouldn't randomly break :)

catch’s picture

Title: Add css_target to navigation libraries » Use aggregate_target for navigation libraries
needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

catch’s picture

Status: Needs work » Reviewed & tested by the community

Rebased.

berdir’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update

Checking this in \Drupal\Core\Asset\CssCollectionGrouper::group, I noticed this causes two separate libraries for navigation to be loaded, first it's all the base category libraries like internal.tooltip, between normalize and system.base. And then later on it is internal.navigation, which uses the theme category. not a big deal, but I'm still confused as to why you'd want to use those css categories, now that we have explicit dependencies. Does it make sense to normalize all these navigation libraries to use the same category? which one? are modules supposed to use "theme", as they are not, in fact, a theme?

Also, this only sets the css aggregate key and not js, is there a reason for this?

The issue summary doesn't say anything about navigation module, just not-themes, seems like this had a possibly wider scope once?

Tagging for needs issue summary update and setting back to needs review at least.

catch’s picture

Status: Needs review » Needs work

And then later on it is internal.navigation, which uses the theme category. not a big deal, but I'm still confused as to why you'd want to use those css categories, now that we have explicit dependencies. Does it make sense to normalize all these navigation libraries to use the same category? which one

Agreed with these questions, but I think we need a front end/CSS person for the answers, which I am not. There's an issue around somewhere to use native CSS layers instead of groups, which would definitely mean we could remove them altogether, but that'd be a big refactor of everything probably.

Also, this only sets the css aggregate key and not js, is there a reason for this?

I split this out of an issue specifically for CSS, but now its on its own, we could add the js libraries in I think - marking needs work for that at least.