Fix CSS minifier selector whitespace - #90
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe CSS minification logic in AssetMinifier was modified to exclude colons from whitespace-stripping punctuation handling and add a dedicated regex for collapsing whitespace after colons. The needsCssSpace helper was simplified using str_contains checks, removing the isIdentifierChar helper. A corresponding test was added. ChangesCSS Minification Whitespace Fix
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adjusts the CSS minification logic to avoid breaking selector semantics and value parsing by preserving whitespace that is syntactically required (e.g., descendant combinators and multi-token values), and adds a regression test to cover these cases.
Changes:
- Split CSS punctuation trimming so colon handling only removes whitespace after
:(avoids breaking selectors like.content :is(...)). - Simplify whitespace-preservation logic in the CSS whitespace-collapser to retain necessary spaces more reliably.
- Add PHPUnit regression coverage for selector/value whitespace preservation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Build/AssetMinifier.php |
Adjusts CSS whitespace/punctuation minification rules to preserve required selector/value whitespace. |
tests/Unit/Build/AssetMinifierTest.php |
Adds regression coverage ensuring minification keeps required whitespace in selectors and multi-token values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $content = self::collapseCssWhitespace($content); | ||
| $content = (string) preg_replace('/\s*([{}:;,>~=\[\]])\s*/', '$1', $content); | ||
| $content = (string) preg_replace('/\s*([{};,>~=\[\]])\s*/', '$1', $content); | ||
| $content = (string) preg_replace('/:\s*/', ':', $content); | ||
| $content = (string) preg_replace('/;}/', '}', $content); |
| public function testMinifiesCssWithoutBreakingSelectorOrValueWhitespace(): void | ||
| { | ||
| $css = <<<'CSS' | ||
| .site-header .container { | ||
| display: grid; | ||
| padding: 0 .125rem; | ||
| } | ||
|
|
||
| .container:has(.docs-layout) { | ||
| max-width: calc(100% - (var(--page-gutter) * 2)); | ||
| } | ||
|
|
||
| .content :is(h1, h2) .header-anchor { | ||
| margin: 1rem 1.25rem; | ||
| } | ||
| CSS; | ||
|
|
||
| assertSame( | ||
| '.site-header .container{display:grid;padding:0 .125rem}.container:has(.docs-layout){max-width:calc(100% - (var(--page-gutter) * 2))}.content :is(h1,h2) .header-anchor{margin:1rem 1.25rem}', | ||
| AssetMinifier::minify('app.css', $css), |
Summary
.content :is(...)remain validVerification
make build-docsmake test -- --filter AssetMinifierTestmake test -- tests/Unit/Build/AssetMinifierTest.php tests/Unit/Build/ThemeAssetCopierTest.php tests/Unit/Build/ContentAssetCopierTest.php tests/Unit/Build/AssetFileWriterTest.phpmake testSummary by CodeRabbit