Skip to content

Scale homepage hero slides correctly on mobile #38740

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.

Sign up for GitHub

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

Merged
pelikhan merged 6 commits into from
Jun 12, 2026
+36 −3
Merged

Scale homepage hero slides correctly on mobile #38740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6a89b76
Plan hero slide scaling fix
Copilot Jun 11, 2026
e21cf92
Fix hero slide scaling on mobile
Copilot Jun 11, 2026
6fc4a83
Tidy hero slide follow-up changes
Copilot Jun 11, 2026
3c76496
Polish hero slide scaling logic
Copilot Jun 11, 2026
6ea4e1a
Refine hero slide sizing cleanup
Copilot Jun 11, 2026
0a0d597
Address hero review feedback
Copilot Jun 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions docs/src/components/WorkflowHero.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ function unmountFromHero() {
let renderVersion = 0;
/** @type {number | undefined} */
let autoplayTimer;
const MINIMUM_STAGE_DIMENSION = 1;

@github-actions github-actions Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnose] MINIMUM_STAGE_DIMENSION = 1 is used to guard the division in the scale formula against a zero-or-negative clientWidth/Height, but that purpose isn't immediately obvious from the name or value.

A brief inline comment would help future readers (and avoid someone bumping it to 0 thinking it's a threshold):

const MINIMUM_STAGE_DIMENSION = 1; // prevents division by zero if stage has no size yet

const MINIMUM_RENDER_SCALE = 0.1;

function stopCarousel() {
window.clearInterval(autoplayTimer);
Expand Down Expand Up @@ -103,8 +105,16 @@ function unmountFromHero() {

const devicePixelRatio = window.devicePixelRatio || 1;
const baseViewport = page.getViewport({ scale: 1 });
const availableWidth = stage.clientWidth - 16;
const scale = Math.max(availableWidth / baseViewport.width, 0.5);
const stageStyles = window.getComputedStyle(stage);
const parseInset = (value) => Number.parseFloat(value) || 0;

@github-actions github-actions Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

parseInset is re-instantiated as a new function object on every render and every resize event — hoist it out of renderPage.

💡 Suggested fix

parseInset closes over nothing and is pure utility. Defining it inside renderPage allocates a new closure on each call (which fires on every slide transition and resize event). Move it alongside the other module-level constants:

const MINIMUM_STAGE_DIMENSION = 1;
const MINIMUM_RENDER_SCALE = 0.1;
const parseInset = (value) => Number.parseFloat(value) || 0;

Then remove the definition from inside renderPage.

const horizontalInset = parseInset(stageStyles.paddingLeft) + parseInset(stageStyles.paddingRight);
const verticalInset = parseInset(stageStyles.paddingTop) + parseInset(stageStyles.paddingBottom);
const availableWidth = Math.max(stage.clientWidth - horizontalInset, MINIMUM_STAGE_DIMENSION);
const availableHeight = Math.max(stage.clientHeight - verticalInset, MINIMUM_STAGE_DIMENSION);

@github-actions github-actions Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If stage.clientHeight is 0 at render time, the height path silently forces scale to MINIMUM_RENDER_SCALE (0.1) — rendering a postage-stamp slide with no error surfaced.

💡 Details

When stage.clientHeight === 0 (e.g., the element was moved via mountIntoHero() and layout hasn't committed yet, or a resize fires during a layout flush):

availableHeight = Math.max(0 - 0, 1) = 1
height ratio    = 1 / ~540            ≈ 0.0019
scale           = Math.max(Math.min(widthRatio, 0.0019), 0.1) = 0.1

The old code only used clientWidth, which is far more stable at the time renderPage is first invoked. The new height path adds a new silent degradation mode.

Suggested mitigations (pick one or combine):

  1. Guard the render: skip (or defer) rendering if availableWidth < threshold || availableHeight < threshold, and retry via requestAnimationFrame.
  2. Log the degrade: at minimum console.warn when either dimension is at MINIMUM_STAGE_DIMENSION so debugging is possible.
  3. Validate MINIMUM_RENDER_SCALE: 0.1 allows a ~100×56 px render for a typical 1000-pt-wide PDF. If this floor is ever actually hit in production, the slide is unreadable. Raise it or surface an error instead.
if (availableWidth <= MINIMUM_STAGE_DIMENSION || availableHeight <= MINIMUM_STAGE_DIMENSION) {
  // Layout not ready; retry next frame
  requestAnimationFrame(() => void renderPage(pageNumber));
  return;
}

const scale = Math.max(
Math.min(availableWidth / baseViewport.width, availableHeight / baseViewport.height),
MINIMUM_RENDER_SCALE,
);
const viewport = page.getViewport({ scale });

bufferCanvas.width = Math.floor(viewport.width * devicePixelRatio);
Expand All @@ -130,7 +140,10 @@ function unmountFromHero() {
canvasContext.drawImage(bufferCanvas, 0, 0);

activePage = pageNumber;
loading?.setAttribute('hidden', 'hidden');
if (loading) {
loading.setAttribute('hidden', 'hidden');

@github-actions github-actions Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnose] The root cause here is that .workflow-hero__loading { display: grid } in the stylesheet overrides the browser's default [hidden] { display: none }, so setAttribute('hidden') alone never hid the overlay.

The JS workaround works, but fixing it in CSS is more idiomatic and removes the coupling between the JS and the display implementation:

.workflow-hero__loading[hidden] { display: none; }

That would make the style.display = 'none' line unnecessary.

loading.style.display = 'none';
}
root.classList.add('is-ready');
}

Expand Down
20 changes: 20 additions & 0 deletions docs/tests/slide-preview.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,26 @@ test.describe('Slide Preview on Homepage', () => {
await expect(slideHero).toHaveClass(/is-ready/);
});

test('should keep the rendered slide within the frame on mobile screens', async ({ page }) => {
await page.setViewportSize({ width: 390, height: 844 });
await page.goto('/gh-aw/');

@github-actions github-actions Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] This test calls page.goto twice — once via beforeEach and again here after resetting the viewport. The first load is wasted and can mask flakiness.

Consider extracting the mobile scenario into its own describe block with test.use({ viewport: { width: 390, height: 844 } }) so the page is only loaded once at the correct size.

await page.waitForLoadState('networkidle');

const loading = page.locator('[data-slide-loading]');
await expect(loading).toBeHidden({ timeout: 10000 });

const stage = page.locator('[data-slide-stage]');
const canvas = page.locator('[data-slide-canvas]');
const stageBox = await stage.boundingBox();
const canvasBox = await canvas.boundingBox();

expect(stageBox).not.toBeNull();
expect(canvasBox).not.toBeNull();

expect(canvasBox!.width).toBeLessThanOrEqual(stageBox!.width);

@github-actions github-actions Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The assertion compares against stageBox.width (the outer element including its padding), but the scale formula guarantees the canvas fits within the padded inner area (clientWidth - horizontalInset). The test passes even if the canvas overlaps the frame's padding.

For a tighter regression guard, compare against the inner slides container ([data-slide-stage] .workflow-hero__slides or subtract the stage's padding from the bounds) so the test would catch a regression where the canvas renders inside the outer border but still clips past the visible frame.

expect(canvasBox!.height).toBeLessThanOrEqual(stageBox!.height);
Comment on lines +49 to +58
});

test('should be keyboard accessible', async ({ page }) => {
// Wait for slides to load
const loading = page.locator('[data-slide-loading]');
Expand Down
Loading