Skip to content

Conversation

@danspringer
Copy link
Member

Noscript für CSS ergänzt für render-blocking-assets, media="all" als default gestrichen, wegen best-practice.
https://developer.chrome.com/docs/lighthouse/performance/render-blocking-resources?hl=de

man kann nun wie folgt css einbinden:

$assets->setCss('plyr', '/assets/addons/plyr/vendor/plyr/dist/plyr.css','', ['rel' => 'preload', 'as' => 'style'], true);

daraus wird dann:

<link rel="preload" as="style" href="/assets/addons/plyr/vendor/plyr/dist/plyr.css" type="text/css" onload="this.onload=null;this.rel='stylesheet'" /><noscript><link rel="stylesheet" href="/assets/addons/plyr/vendor/plyr/dist/plyr.css"></noscript>

Noscript für CSS ergänzt für render-blocking-assets, media="all" als default gestrichen, wegen best-practice
Noscript für CSS ergänzt für render-blocking-assets, media="all" als default gestrichen, wegen best-practice
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds <noscript> support for CSS assets to avoid render-blocking and removes the default media="all" to follow best practices for loading CSS.

  • Introduces a noscript flag in getLinkTag() to wrap stylesheets in <noscript> tags with preload and onload attributes.
  • Changes setCss() to require explicit media values (no default) and to accept the new noscript parameter.
  • Updates getCss() to pass the noscript flag through to the link tag renderer.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
lib/theme_assets_trait.php Extended getLinkTag() with a noscript parameter and onload logic.
lib/theme_assets.php Modified setCss() and getCss() to remove default media and support noscript.
Comments suppressed due to low confidence (3)

lib/theme_assets_trait.php:41

  • The docblock above getLinkTag() needs to be updated to include the new @param bool $noscript so that the purpose and default behavior of this parameter are clear.
    private function getLinkTag(string $key, string $file, array $attributes, string $cache_buster = '', bool $noscript = false): string

lib/theme_assets.php:66

  • This change is a breaking API update: removing the default media='all' and adding the $noscript parameter will affect existing callers. Consider documenting this change clearly or providing a migration path.
    public function setCss(string $key, string $data, string $media = '', array $attributes = [], bool $noscript = false): theme_assets

lib/theme_assets.php:66

  • Update the docblock for setCss() to describe the new $noscript parameter and note that the media parameter no longer defaults to 'all'.
    public function setCss(string $key, string $data, string $media = '', array $attributes = [], bool $noscript = false): theme_assets

$attributes['type'] = $attributes['type'] ?? 'text/css';

return '<link'.rex_string::buildAttributes($attributes).' />'.PHP_EOL;
return '<link'.rex_string::buildAttributes($attributes).$onload.' />'.$noscript_tag.PHP_EOL;
Copy link

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider inserting a newline or space before $noscript_tag so that the <noscript> element is rendered on its own line for improved readability of the generated HTML.

Suggested change
return '<link'.rex_string::buildAttributes($attributes).$onload.' />'.$noscript_tag.PHP_EOL;
return '<link'.rex_string::buildAttributes($attributes).$onload.' />'.PHP_EOL.$noscript_tag.PHP_EOL;

Copilot uses AI. Check for mistakes.
@DanielWeitenauer
Copy link
Member

Hi,
da hatte ich tatsächlich vorher noch nichts von gelesen.
Generell finde ich solch eine Ergänzung interessant.

Allerdings finde ich die Anpassung so noch nicht ideal:

  • es gibt einen, wenn auch kleinen BC break, wenn das media-Attribute nun anders vorbelegt wird.
  • ich bin kein großer Fan von langen Parameter-Ketten, besonders wenn es sich nur um Flags handelt, die Zahl der Parameter ist sowieso schon sehr hoch.
  • ich denke, dass solche Spezialfälle nicht bis in den Trait durchschlagen sollten

Soweit ich mir das auf die Schnelle anlesen konnte, gibt es ein Best-Practise-Setting, mit dem man das Thema dann standardisiert angehen kann.
Daher folgende Vorschläge für eine Anpassung:

  1. eine neue Methode einführen, z.B. setCssAsync, die die alle Best-Practise-Parameter vorbelegt hat
  2. oder - und das ist momentan mein Favorit - eine pluggable Erweiterung ähnlich theme_minify anlegen, die sich in die Extension Points einhängt und alle link-Tags anpasst, ohne dass man vorhandenen Code groß ändern muss. Sie könnte, wie theme_minify zusätzlich pro Instanz über ein setAction-Setting gesteuert werden. Das würde den Basicode weiter schlank halten und sich leicht in vorhandene Setups einfügen lassen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants