-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: let the doc bottom navigation wrap on narrow viewports #2488
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
base: main
Are you sure you want to change the base?
Changes from all commits
b83ad71
be47297
a098c31
4406f89
a115d68
2bb4d11
954257f
6fed27b
953ba44
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 |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| import { test, expect } from '@playwright/test'; | ||
|
|
||
| // Regression coverage for https://github.com/expressjs/expressjs.com/issues/2486 | ||
| const DOC_PATH = '/en/guide/migrating-4/'; | ||
|
|
||
| test.describe('Doc bottom navigation', () => { | ||
| test('should not overflow horizontally on a narrow viewport', async ({ page }) => { | ||
| await page.setViewportSize({ width: 400, height: 900 }); | ||
| await page.goto(DOC_PATH); | ||
|
|
||
| const nav = page.locator('.doc-nav'); | ||
| await expect(nav).toBeVisible(); | ||
|
|
||
| const overflow = await nav.evaluate((el) => el.scrollWidth - el.clientWidth); | ||
| expect(overflow).toBeLessThanOrEqual(1); | ||
|
Check failure on line 15 in tests/e2e/doc-bottom-nav.spec.ts
|
||
| }); | ||
|
|
||
| test('should move the next link above the previous link when they cannot share a line', async ({ | ||
| page, | ||
| }) => { | ||
| await page.setViewportSize({ width: 400, height: 900 }); | ||
| await page.goto(DOC_PATH); | ||
|
|
||
| const prev = page.locator('.doc-nav__link--prev'); | ||
| const next = page.locator('.doc-nav__link--next'); | ||
|
|
||
| await expect(prev).toBeVisible(); | ||
| await expect(next).toBeVisible(); | ||
|
|
||
| const prevBox = await prev.boundingBox(); | ||
| const nextBox = await next.boundingBox(); | ||
|
|
||
| if (!prevBox || !nextBox) { | ||
| throw new Error('expected both doc nav links to be laid out'); | ||
| } | ||
|
|
||
| expect(nextBox.y + nextBox.height).toBeLessThanOrEqual(prevBox.y); | ||
|
Check failure on line 37 in tests/e2e/doc-bottom-nav.spec.ts
|
||
| }); | ||
|
|
||
| test('should keep both links on a single line on a wide viewport', async ({ page }) => { | ||
| await page.setViewportSize({ width: 1400, height: 900 }); | ||
| await page.goto(DOC_PATH); | ||
|
|
||
| const prevBox = await page.locator('.doc-nav__link--prev').boundingBox(); | ||
| const nextBox = await page.locator('.doc-nav__link--next').boundingBox(); | ||
|
|
||
| if (!prevBox || !nextBox) { | ||
| throw new Error('expected both doc nav links to be laid out'); | ||
| } | ||
|
|
||
| expect(nextBox.y).toBeCloseTo(prevBox.y, 0); | ||
| expect(nextBox.x).toBeGreaterThan(prevBox.x); | ||
| }); | ||
| }); | ||
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.
I don’t think these tests are particularly useful, as we generally don’t test layout behaviours such as “the next link should move above the previous link when they can’t fit on the same line” or “the content should not overflow horizontally on a narrow viewport.”
These are more like responsive/layout implementation details rather than behaviours we need to explicitly test. I’d suggest removing these tests to keep the test suite focused on meaningful and maintainable behaviours.