-
Notifications
You must be signed in to change notification settings - Fork 1
fix(layout): make .sf-bento and .sf-grid-cols-N actually respond to their container #607
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
cfdd702
a3afdbe
ac232f4
55b8e2c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -644,6 +644,140 @@ test.describe('layout: .sf-bento', () => { | |
| ); | ||
| expect(cols).toBe(2); | ||
| }); | ||
|
|
||
| // .sf-bento's own @container breakpoints must resolve against an ANCESTOR | ||
| // container, not itself — a size container cannot be the subject of its | ||
| // own @container query (SL-034); .sf-bento used to declare `container` | ||
| // on itself, which made these breakpoints a permanent silent no-op at | ||
| // every viewport, mobile included. | ||
| test('collapses to 1 column below the mobile breakpoint (ancestor container)', async ({ page }) => { | ||
| await setup(page, ` | ||
| <div style="container-type:inline-size; width:300px"> | ||
| <div id="g" class="sf-bento"><div>A</div><div>B</div></div> | ||
| </div> | ||
| `); | ||
| const cols = await page.locator('#g').evaluate(el => | ||
| getComputedStyle(el).gridTemplateColumns.split(' ').length | ||
| ); | ||
| expect(cols).toBe(1); | ||
| }); | ||
|
|
||
| test('a spanning child does not force a phantom 2nd column at the mobile breakpoint', async ({ page }) => { | ||
| await setup(page, ` | ||
| <div style="container-type:inline-size; width:300px"> | ||
| <div class="sf-bento"> | ||
| <div id="w" class="sf-bento-wide">wide</div> | ||
| <div>B</div> | ||
| </div> | ||
| </div> | ||
| `); | ||
| const [gridWidth, itemWidth] = await Promise.all([ | ||
| page.locator('.sf-bento').evaluate(el => el.getBoundingClientRect().width), | ||
| page.locator('#w').evaluate(el => el.getBoundingClientRect().width), | ||
| ]); | ||
| expect(itemWidth).toBeLessThanOrEqual(gridWidth + 1); | ||
| }); | ||
|
|
||
| test('--featured (span 2/2) also does not force a phantom 2nd column at the mobile breakpoint', async ({ page }) => { | ||
| await setup(page, ` | ||
| <div style="container-type:inline-size; width:300px"> | ||
| <div class="sf-bento"> | ||
| <div id="f" class="sf-bento-featured">featured</div> | ||
| <div>B</div> | ||
| </div> | ||
| </div> | ||
| `); | ||
| const [gridWidth, itemWidth] = await Promise.all([ | ||
| page.locator('.sf-bento').evaluate(el => el.getBoundingClientRect().width), | ||
| page.locator('#f').evaluate(el => el.getBoundingClientRect().width), | ||
| ]); | ||
| expect(itemWidth).toBeLessThanOrEqual(gridWidth + 1); | ||
| }); | ||
|
|
||
| test('uses the wider column count once past the mobile breakpoint', async ({ page }) => { | ||
| await setup(page, ` | ||
| <div style="container-type:inline-size; width:1600px"> | ||
| <div id="g" class="sf-bento"><div>A</div><div>B</div></div> | ||
| </div> | ||
| `); | ||
| const cols = await page.locator('#g').evaluate(el => | ||
| getComputedStyle(el).gridTemplateColumns.split(' ').length | ||
| ); | ||
| expect(cols).toBeGreaterThan(1); | ||
| }); | ||
|
|
||
| // `em` inside an @container condition resolves against the container's | ||
| // (fluid, viewport-driven) inherited font-size here — not a fixed 16px — | ||
| // so the effective pixel breakpoint isn't identical across engines. 750px | ||
| // sits comfortably between every observed 30em/48em interpretation; 900px | ||
| // was measured to land right on that boundary and flipped between 2 and 4 | ||
| // columns depending on the browser (see the grid-cols-4 test below). | ||
| test('uses 2 columns in the mid-range between 30em and 48em', async ({ page }) => { | ||
| await setup(page, ` | ||
| <div style="container-type:inline-size; width:750px"> | ||
| <div id="g" class="sf-bento"><div>A</div><div>B</div></div> | ||
| </div> | ||
| `); | ||
| const cols = await page.locator('#g').evaluate(el => | ||
| getComputedStyle(el).gridTemplateColumns.split(' ').length | ||
| ); | ||
| expect(cols).toBe(2); | ||
| }); | ||
| }); | ||
|
|
||
| // ── .sf-grid-cols-2/3/4/6 ────────────────────────────────────────── | ||
| test.describe('layout: .sf-grid-cols-2/3/4/6', () => { | ||
| // Same SL-034 constraint as .sf-bento above: these used to establish and | ||
| // then query their own container, so the breakpoint below was dead code | ||
| // at every viewport — only an ancestor container makes it fire. | ||
| // A width of 900px (56.25em at a fixed 16px basis) originally lived here | ||
| // and was flaky in CI: `em` inside an @container condition resolves | ||
| // against the container's inherited font-size, which here is the fluid | ||
| // `--sf-text-m` (~19px at this viewport, not 16px), and engines don't | ||
| // agree on the resulting effective breakpoint down to the pixel — 900px | ||
| // sat close enough to the 48em boundary that Chromium read it as past the | ||
| // breakpoint while Firefox/WebKit read it as short of it. 1600px clears | ||
| // every observed interpretation. | ||
| test('resolves its column count against an ancestor container, not itself', async ({ page }) => { | ||
| await setup(page, ` | ||
| <div style="container-type:inline-size; width:1600px"> | ||
| <div id="g" class="sf-grid-cols-4"><div>1</div><div>2</div><div>3</div><div>4</div></div> | ||
| </div> | ||
| `); | ||
| const cols = await page.locator('#g').evaluate(el => | ||
| getComputedStyle(el).gridTemplateColumns.split(' ').length | ||
| ); | ||
| expect(cols).toBe(4); | ||
| }); | ||
|
|
||
| test('stays 1 column below the breakpoint', async ({ page }) => { | ||
| await setup(page, ` | ||
| <div style="container-type:inline-size; width:300px"> | ||
| <div id="g" class="sf-grid-cols-4"><div>1</div><div>2</div><div>3</div><div>4</div></div> | ||
| </div> | ||
| `); | ||
| const cols = await page.locator('#g').evaluate(el => | ||
| getComputedStyle(el).gridTemplateColumns.split(' ').length | ||
| ); | ||
| expect(cols).toBe(1); | ||
| }); | ||
|
Comment on lines
+753
to
+763
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
At 300px, no |
||
|
|
||
| // Below 30em NEITHER @container rule matches, so gridTemplateColumns falls | ||
| // back to the unset "none" (still 1 "column" by construction) — the case | ||
| // above alone can't distinguish a working ancestor container from a | ||
| // completely absent one. This mid-range case only passes if the 30em | ||
| // breakpoint's ancestor-container query actually fired. | ||
| test('uses the 2-column mid breakpoint between 30em and 48em', async ({ page }) => { | ||
| await setup(page, ` | ||
| <div style="container-type:inline-size; width:750px"> | ||
| <div id="g" class="sf-grid-cols-4"><div>1</div><div>2</div><div>3</div><div>4</div></div> | ||
| </div> | ||
| `); | ||
| const cols = await page.locator('#g').evaluate(el => | ||
| getComputedStyle(el).gridTemplateColumns.split(' ').length | ||
| ); | ||
| expect(cols).toBe(2); | ||
| }); | ||
| }); | ||
|
|
||
| // ── .sf-alternate ─────────────────────────────────────────────── | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.sf-bento-featuredmobile overflow not exercisedThe CSS fix resets both
.sf-bento-wideand.sf-bento-featuredtospan 1at the mobile breakpoint, but only.sf-bento-wideis covered by this test. Afeaturedchild carriesgrid-column: span 2; grid-row: span 2;, so an item-width assertion analogous to thesf-bento-widecase would confirm itsspan 2is also being overridden correctly and not creating a phantom column.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!