Skip to content

Different units and dynamic variable values#4677

Open
davidwebca wants to merge 3 commits intoalpinejs:mainfrom
davidwebca:main
Open

Different units and dynamic variable values#4677
davidwebca wants to merge 3 commits intoalpinejs:mainfrom
davidwebca:main

Conversation

@davidwebca
Copy link

@davidwebca davidwebca commented Sep 17, 2025

I extended the functionality of the existing collapse plugin to allow using any known CSS unit or any CSS variable.

Ex.:
x-collapse.min.var(--collapse-min) will now work.
x-collapse.min.100lvh will now work.
x-collapse.min.100 still fallbacks to pixels.

The variable detection hence allows Tailwind to set arbitrary values to variables with responsive variants.

<div class="[--collapse-min:100px] lg:[--collapse-min:500px]" x-collapse.min.var(--collapse-min)>
    Example content
</div>

The example above allows us to set a minimum height to 100px on mobile and to 500px on desktop (lg screen size in Tailwind).

Maybe issue? : parentheses aren't technically in the spec for HTML attributes, but supported in all major browsers.

Adresses #2282, #2984 and might invalidate #4547

davidwebca and others added 3 commits September 17, 2025 00:52
I extended the functionality of the existing collapse plugin to allow using any known CSS unit or any CSS variable.

Ex.:
```x-collapse.min.var(--collapse-min)``` will now work.
```x-collapse.min.100lvh``` will now work.
```x-collapse.min.100``` still fallbacks to pixels.

The variable detection hence allows Tailwind to set arbitrary values to variables with responsive variants.

```
<div class="[--collapse-min:100px] lg:[--collapse-min:500px]" x-collapse.min.var(--collapse-min)>
    Example content
</div>
```

The example above allows us to set a minimum height to 100px on mobile and to 500px on desktop (lg screen size in Tailwind).

Maybe issue? : parentheses aren't technically in the spec for HTML attributes, but supported in all major browsers.
… add tests

- Reverted all gratuitous semicolons and style reformatting
- Fixed regex that wouldn't match single-digit values (e.g. 5px)
- Fixed unitMatch check that could never be falsy
- Fixed null dereference when match fails
- Fixed string concatenation bug in transition start height
- Changed floor default from 0 to '0px' for consistent string type
- Simplified modifierValue parsing logic
- Added tests for unitless values and CSS variables

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@calebporzio
Copy link
Collaborator

PR Review: #4677 — Different units and dynamic variable values

Type: Feature
Verdict: Needs discussion

What's happening (plain English)

Currently, x-collapse.min only supports pixel values: x-collapse.min.100px or x-collapse.min.100. This PR adds support for:

  1. Any CSS unit: x-collapse.min.50vh, x-collapse.min.10rem, etc.
  2. CSS variables: x-collapse.min.var(--collapse-min) — which enables responsive minimum heights when paired with Tailwind's responsive variants (e.g. class="[--collapse-min:100px] lg:[--collapse-min:500px]").

The core change is small: instead of floor being a raw number that gets px appended everywhere, it's now always a string with units included (e.g. '0px', '50vh', 'var(--x)').

Other approaches considered

  1. Current approach (modify modifierValue to return units-included strings): This is the simplest — just change how the value is parsed and make the rest of the code use it directly. This is what I implemented.
  2. Separate x-collapse.min-var modifier: Would avoid the parentheses-in-HTML-attributes concern the contributor raised, but adds unnecessary API surface.
  3. Expression-based approach (e.g. x-collapse.min="someExpression"): Would be more flexible but breaks Alpine's modifier-based pattern for this directive entirely.

The modifier approach is the right call — it's consistent with how x-collapse already works.

Changes Made

I substantially rewrote the contributor's implementation. The original PR had several issues:

  1. Style violations: Added semicolons to every line and changed ! spacing to ! throughout the entire file. Alpine uses no semicolons and ! with a space. I reverted all of this.
  2. Bug: regex wouldn't match single-digit values: The pattern [0-9]+\.?[0-9]+ requires at least 2 digits, so 5px wouldn't match. Fixed to [0-9]+(?:\.[0-9]+)?.
  3. Bug: null dereference: If the regex didn't match, match[1].startsWith(...) would throw. Added a startsWith guard before regex.
  4. Bug: dead code path: The unitMatch check (if (!unitMatch)) could never be falsy because the regex had ? making the unit optional — empty string always matches. Simplified the whole thing to a single regex with fallback.
  5. Bug: string concatenation in transitions: When floor was a string like '25px' and current = floor, the code did current + 'px' producing '25pxpx'. Fixed by using a startHeight variable that uses floor directly when current === full.
  6. Bug: broken default case: The default floor = 0 (number) was used as el.style.height = floor which sets el.style.height = 0 — a no-op in CSS. Changed default to '0px'.
  7. constlet: Alpine convention.
  8. Missing tests: Added tests for unitless values and CSS variables.

Test Results

  • All 5 existing collapse tests: PASS
  • 2 new tests (unitless values, CSS variables): PASS
  • Regression verified: CSS variable test FAILS on main, passes with fix
  • CI: was failing before my changes (contributor's code had build-breaking issues)

Code Review

The final implementation is clean and minimal — only 3 lines changed in the main collapse function (default value, initial height assignment, and transition start height), plus ~6 lines in modifierValue for the new parsing logic.

Concern about parentheses in HTML attributes: The contributor noted that var(--name) uses parentheses which aren't technically in the HTML spec for attribute values. All major browsers handle this fine, but it's worth noting. Alpine's modifier parser handles it because . is the delimiter and parentheses are preserved within the modifier value.

Scope question: This is a nice-to-have feature with modest community demand (2 rocket reactions, references 3 issues). The implementation is small and non-breaking. But it does expand the API surface of the collapse plugin.

Security

No security concerns identified.

Verdict

Needs discussion — the implementation is solid after my fixes, and the feature has a real use case (responsive collapse heights with Tailwind). But I want you to weigh in on:

  1. Is this feature worth the added complexity? It's small (~6 lines of parsing logic), but it's new API surface to maintain.
  2. Are you comfortable with var() in modifier values? The parentheses work in practice but are unconventional for HTML attributes.
  3. The contributor's PR branch is main — they branched from their fork's main, which is a minor red flag for contribution hygiene but doesn't affect the code.

If you're good with the feature, it's ready to merge. I've fixed all the issues and added tests.


Reviewed by Claude

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