Skip to content

Add native grid-lanes entry point#681

Open
glen-cheney wants to merge 12 commits intomainfrom
feat/grid-lanes
Open

Add native grid-lanes entry point#681
glen-cheney wants to merge 12 commits intomainfrom
feat/grid-lanes

Conversation

@glen-cheney
Copy link
Copy Markdown
Owner

@glen-cheney glen-cheney commented Apr 23, 2026

  • Add a new entry point & css file: /grid-lanes and /grid-lanes.css
  • Add 3 new docs pages for Grid Lanes
  • Extract core logic to a shared core directory
  • Update the homepage demo to be able to toggle between the "classic" Shuffle and the new Grid Lanes version.

Closes #660

glen-cheney and others added 4 commits April 22, 2026 22:51
* start phase 1

Co-authored-by: Copilot <copilot@github.com>

* Fix (and upgrade) lint

---------

Co-authored-by: Copilot <copilot@github.com>
* Filtering and sorting

* fix type-check

Co-authored-by: Copilot <copilot@github.com>

---------

Co-authored-by: Copilot <copilot@github.com>
@glen-cheney glen-cheney linked an issue Apr 25, 2026 that may be closed by this pull request
glen-cheney and others added 8 commits April 25, 2026 23:09
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
@glen-cheney
Copy link
Copy Markdown
Owner Author

Code Review — PR #681: shufflejs Grid Lanes entry point

53 files, +4,726/−869. Overall: solid implementation. The architecture is clean, the test suite is broad, and the core extraction is safe. Two documentation bugs and the committed .vscode/ file are the most important things to fix before the beta.


Critical

1. .vscode/settings.json should not ship (packages/shuffle/.vscode/ or root)

git diff --stat confirms .vscode/settings.json was added. Contents:

{ "chat.tools.terminal.autoApprove": { "npx tsc": true } }

This is a local IDE setting. It should be in .gitignore, not committed. It leaks developer toolchain preferences to consumers who npm pack / clone the repo.

Fix: rm .vscode/settings.json, add .vscode/ to .gitignore (or the root .gitignore if one exists).


Recommended

2. add() docs contradict implementation (apps/website/docs/grid-lanes/configuration.md:145)

The docs say:

New elements must be children of the container before calling add().

But the implementation (grid-lanes.ts:475):

if (element.parentElement !== this.element) {
  this.element.append(element);
}

add() auto-appends elements that aren't already children. The doc constraint is incorrect — consumers who follow it are fine, but consumers who call add([el]) before appending will also be fine, which is inconsistent with what the docs promise. Pick one: either enforce the precondition (throw if element isn't a child) or update the docs to say "elements will be appended if they are not already children."


3. sort({}) doesn't restore original order — docs say it does (configuration.md:132)

The docs say:

Pass null or an empty object to restore original DOM order.

Only null works. Trace:

  • sort({})this.lastSort = {} (truthy)
  • #sortItems: if (!this.lastSort) → false → falls into sorter(items, {})
  • sorter with empty opts: no by, no compare, no randomize → returns the array in its current order, not original encounter order.

sort(null) correctly hits the !this.lastSort branch and sorts by defaultOrder. The docs should say "Pass null (not {})". The test suite only tests sort(null), which is why this wasn't caught (grid-lanes.sort-visibility.test.ts:48).


4. #validateDisplay() silently destroys the instance for off-DOM elements (grid-lanes.ts:130–135)

if (!this.#validateDisplay()) {
  console.error('GridLanes container must use `display: grid` or `display: grid-lanes`.');
  this.destroy();
  return;
}

#validateDisplay() calls globalThis.getComputedStyle(this.element).display. For an element not yet in the document, getComputedStyle returns display: "" (empty string), which fails the check. The constructor silently calls destroy() and returns, leaving the caller with a non-functional instance and no thrown exception.

Users who construct GridLanes on a detached element (valid pattern: build the DOM, construct, then insert) will hit this. The destroy() path is safe here (fields are initialized, nothing has been mutated), but the failure mode is hard to debug.

Suggested fix: throw instead of silently destroying, or add a note to the getting-started docs that the container must be in the live document.


Optional

5. filter(fn) with a zero-parameter function resets to 'all' (grid-lanes.ts:410)

let nextCategory = category;
if (!nextCategory || nextCategory.length === 0) {
  nextCategory = ALL_ITEMS;
}

Function.prototype.length is the number of declared parameters. A zero-arg predicate (filter(() => true)) has .length === 0, so the guard resets it to ALL_ITEMS (showing everything). This is inherited from classic Shuffle and is likely never hit in practice (filter predicates always take at least one argument by convention), but it's a latent footgun if anyone writes filter(() => someCondition).


6. Missing test: update({ force: true }) while disabled

grid-lanes.api.test.ts:182 tests that filter, sort, update, and layout do nothing when disabled — but it calls instance.update() without { force: true }. The force path is untested. Low risk (the code path is a single if (!this.isEnabled && !force) guard), but worth adding for completeness.


Architecture / correctness sign-off

Core extraction (src/core/): The move of Classes, ALL_ITEMS, FILTER_ATTRIBUTE_KEY, FilterMode, EventType, matchesFilter, sorter, and resolveElementOption is clean. The classic Shuffle entry point correctly re-imports them from ./core/. The #doesPassFilter in shuffle.ts correctly delegates to matchesFilter() with the same semantics as the original inline implementation. ✓

resolveElementOption behavior parity: The old #getElementOption for the main container string case was this.element ? this.element.querySelector() : document.querySelector(). At call time this.element wasn't yet set (undefined → falsy), so it always used document.querySelector. The new resolveElementOption(element) without root defaults to document. Same behavior. The option[0] ?? null jQuery fix (returning null for empty jQuery objects instead of undefined) is a strict improvement. ✓

package.json exports: The exports map, types, files, and sideEffects are all correct. "sideEffects": ["./dist/shuffle-lanes.css"] is right (CSS has side effects; JS entries are side-effect-free). ✓

tsdown.config.ts: Two configs — one for shuffle.ts, one for shuffle-lanes.ts — both ESM-only, matching the exports map. Output names (shuffle.mjs, shuffle-lanes.mjs) align with the exports paths. ✓

Module-level itemIdCounter/defaultOrderCounter (grid-lanes.ts:86–87): Global across instances, monotonically increasing. This is correct for view-transition-name uniqueness (names must be globally unique per-page). defaultOrder within a single instance still correctly preserves insertion order for sort(null). ✓

View Transitions + view-transition-class: init() sets view-transition-class: 'shuffle-item' which is required for ::view-transition-group(.shuffle-item) to match. This is a Level 2 VT API. Since grid-lanes is itself a 2026+ feature, the assumption that view-transition-class is supported in the same browser population is reasonable. ✓

prefers-reduced-motion in CSS: Present and correct — sets animation-duration: 0.001ms (not 0 to still trigger transition callbacks). ✓

shuffle:removed event ordering: remove() uses this.once(EventType.LAYOUT, ...) inside the remove handler, so shuffle:removed fires after shuffle:layout in the same microtask. Tests verify the ordering at grid-lanes.api.test.ts:120–134. ✓


Score

Category Score
Logic & correctness 9/10
Code quality 9/10
Maintainability 9/10
Efficiency 10/10
Test coverage 8/10
Security 10/10

Beta blockers: 1 (.vscode/settings.json). The two doc bugs are worth fixing before GA but won't cause runtime failures.

@glen-cheney
Copy link
Copy Markdown
Owner Author

GridLanes View Transition Styling Options

Core Issues

GridLanes needs to solve three related View Transition problems:

  1. Runtime animation options do not reach View Transition pseudo-elements.
    GridLanes currently writes values like --shuffle-speed to the grid
    container, but ::view-transition-* pseudo-elements are document-level and
    do not inherit from that container. As a result, options such as speed can
    appear to have no effect.

  2. View Transition snapshots can block page interactivity while they animate.
    The top-level ::view-transition overlay sits above the page during an
    active transition. If it receives pointer events, clicks on filter buttons
    can be swallowed while a transition is running. Related: https://www.bram.us/2025/01/29/view-transitions-page-interactivity/

  3. The grid container height should animate when filtering changes its size.
    Individual items already participate in View Transitions, but the container
    itself also needs a stable view-transition-name if its width/height changes
    should animate as part of the same transition.

Any solution should preserve per-instance runtime options, avoid leaking library
styles into unrelated app transitions, and keep transition cleanup predictable.

Options

This note compares options for getting runtime GridLanes animation settings
(speed, easing, stagger values) into View Transition pseudo-elements.

View Transition pseudo-elements are document-level, so custom properties set on
the grid container are not enough by themselves.

1. Root Inline Styles

Set runtime custom properties directly on document.documentElement.style before
startViewTransition(), then restore the previous values afterward.

Pros:

  • Simple implementation.
  • Supports per-instance runtime values.
  • No stylesheet creation or CSSOM rule management.

Cons:

  • Mutates <html style="">.
  • Easy to leak root styles if cleanup misses an edge case.
  • Feels global even when the transition is library-owned.
  • Can interfere with app code that also manages root inline custom properties.

2. Temporary Stylesheet Per Transition

Inject a scoped <style> element before each transition and remove it after the
transition finishes.

Pros:

  • Avoids inline styles on <html>.
  • Can scope rules with :root:active-view-transition-type(shuffle-lanes).
  • Supports per-instance runtime values.
  • Cleanup removes all transition-specific CSS.

Cons:

  • Creates and removes a stylesheet per transition.
  • CSSOM/style recalculation churn may matter during rapid interactions.
  • More moving parts than root inline styles.

3. Singleton Scoped Stylesheet Rule

Create one module-level scoped CSS rule lazily, keep it for the page lifetime,
and update its custom properties before each GridLanes transition.

Pros:

  • Avoids repeated stylesheet injection/removal.
  • Avoids inline styles on <html>.
  • Supports per-instance runtime values.
  • Rule is inert outside :active-view-transition-type(shuffle-lanes).
  • Fits the platform model: only one document View Transition can be active.

Cons:

  • Leaves a library-owned stylesheet/rule in the document.
  • Needs small singleton/CSSOM management code.
  • Last transition wins, which is fine for document-level View Transitions but
    should be intentional.

Recommendation: this is the best balance for GridLanes.

4. Static CSS Only

Put all timing values in the shipped shuffle-lanes.css file and avoid runtime
CSS mutation entirely.

Pros:

  • No runtime style injection.
  • No root inline styles.
  • Very simple and predictable CSS.

Cons:

  • Cannot fully support per-instance speed, easing, or stagger options.
  • Users would need to override global CSS variables themselves.
  • Does not solve the original speed option bug for runtime configuration.

5. Web Animations API

Start the View Transition, then drive the relevant pseudo-elements with the Web
Animations API instead of relying on CSS custom properties.

Pros:

  • Avoids dynamic stylesheet generation for timing values.
  • Can express runtime durations directly in JavaScript.
  • Potentially gives fine-grained control over animations.

Cons:

  • More complex than CSS-based View Transition styling.
  • Pseudo-element animation support and ergonomics are more fragile.
  • Harder to keep reveal/conceal/stagger behavior readable.
  • More likely to diverge from the shipped CSS users can inspect and override.

@glen-cheney
Copy link
Copy Markdown
Owner Author

Maybe check moveBefore support to use that instead of append. Retains focus and node state.

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.

Explore CSS grid-lanes

1 participant