Skip to content

feat(bento): Phase Ic — responsive polish and visual regression tests #75

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

Open
4444J99 wants to merge 1 commit into
base: main
Choose a base branch
from
+87 −5
Open

feat(bento): Phase Ic — responsive polish and visual regression tests #75

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
73 changes: 73 additions & 0 deletions src/__tests__/bento-visual.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { existsSync, readFileSync } from 'fs';
import { resolve } from 'path';
import { describe, expect, it } from 'vitest';

const DIST = resolve(process.cwd(), 'dist');

describe('bento grid structure', () => {
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Mar 27, 2026

Choose a reason for hiding this comment

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

suggestion: Reuse the built HTML instead of reading index.html from disk in every test

Each test re-reads dist/index.html, adding unnecessary I/O and duplication. Consider loading it once in a beforeAll (e.g., into a shared let html: string) and reusing it across tests to keep them focused on structure and simplify future changes (like switching entry files).

Suggested implementation:

import { existsSync, readFileSync } from 'fs';
import { resolve } from 'path';
import { beforeAll, describe, expect, it } from 'vitest';

const DIST = resolve(process.cwd(), 'dist');

describe('bento grid structure', () => {
	const indexPath = resolve(DIST, 'index.html');
	let html: string;

	beforeAll(() => {
		html = readFileSync(indexPath, 'utf-8');
	});

	it('dist/index.html exists', () => {
		expect(existsSync(indexPath)).toBe(true);
	});

	it('has bento-grid section', () => {

Anywhere later in this file that calls readFileSync(indexPath, ...) or otherwise re-reads dist/index.html should be updated to use the shared html variable instead. For example, replace const html = readFileSync(indexPath, 'utf-8'); with uses of the top-level html defined in the describe block (e.g., directly passing html into the DOM parser or string matchers).

const indexPath = resolve(DIST, 'index.html');

it('dist/index.html exists', () => {
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 27, 2026

Choose a reason for hiding this comment

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

medium

To improve test performance, the index.html file should be read only once for the entire test suite. Move the readFileSync call to the describe block scope so that html is available to all it blocks without redundant file reads.

  const indexPath = resolve(DIST, 'index.html');
  const html = readFileSync(indexPath, 'utf8');

  it('dist/index.html exists', () => {

expect(existsSync(indexPath)).toBe(true);
});

it('has bento-grid section', () => {
const html = readFileSync(indexPath, 'utf8');
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 27, 2026

Choose a reason for hiding this comment

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

medium

Reading the index.html file repeatedly within each it block can lead to unnecessary I/O operations and slow down test execution. It's more efficient to read the file once at the beginning of the describe block and reuse its content across all tests in that suite.

  it('has bento-grid section', () => {
    expect(html).toContain('class="bento-grid"');

expect(html).toContain('class="bento-grid"');
expect(html).toContain('aria-label="Portfolio overview"');
});

it('has hero cell with name and stat pills', () => {
const html = readFileSync(indexPath, 'utf8');
expect(html).toContain('bento-cell--hero');
expect(html).toContain('Anthony James Padavano');
expect(html).toContain('stat-pill');
});

it('has 2 featured project cards with transition:name', () => {
const html = readFileSync(indexPath, 'utf8');
const featured = html.match(/bento-card--featured/g);
expect(featured?.length).toBe(2);
expect(html).toContain('data-astro-transition-scope');
});
Comment on lines +27 to +32
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Mar 27, 2026

Choose a reason for hiding this comment

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

suggestion (testing): Tighten featured cards assertion to ensure the transition scope is applied specifically to featured cards

This test only checks that data-astro-transition-scope exists somewhere in the HTML, so it would still pass if the attribute moved off the featured cards. Please assert that it’s present specifically on the bento-card--featured elements (or that the count of data-astro-transition-scope matches the number of featured cards) so the test better reflects the intended behavior.

Suggested change
it('has 2 featured project cards with transition:name', () => {
const html = readFileSync(indexPath, 'utf8');
const featured = html.match(/bento-card--featured/g);
expect(featured?.length).toBe(2);
expect(html).toContain('data-astro-transition-scope');
});
it('has 2 featured project cards with transition:name', () => {
const html = readFileSync(indexPath, 'utf8');
// Count all featured cards
const featured = html.match(/bento-card--featured/g) ?? [];
expect(featured.length).toBe(2);
// Ensure each featured card has a data-astro-transition-scope attribute
const featuredWithTransitionScope =
html.match(/<[^>]*class="[^"]*\bbento-card--featured\b[^"]*"[^>]*data-astro-transition-scope[^>]*>/g) ?? [];
expect(featuredWithTransitionScope.length).toBe(featured.length);
});


it('has controls cell with view toggle and depth control', () => {
const html = readFileSync(indexPath, 'utf8');
expect(html).toContain('controls-cell');
expect(html).toContain('bento-view-toggle');
expect(html).toContain('data-shibui-control');
});

it('has persona cell with 4 persona links', () => {
const html = readFileSync(indexPath, 'utf8');
expect(html).toContain('persona-cell');
const personaLinks = html.match(/persona-cell__link/g);
expect(personaLinks?.length).toBeGreaterThanOrEqual(4);
Comment on lines +41 to +45
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Mar 27, 2026

Choose a reason for hiding this comment

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

question (testing): Clarify whether exactly 4 personas are expected and assert accordingly

The test name implies exactly 4 persona links, but the assertion uses toBeGreaterThanOrEqual(4). Please either change the assertion to toBe(4) if exactly four are required, or update the test name to indicate an "at least 4" expectation.

});

it('has CTA cell', () => {
const html = readFileSync(indexPath, 'utf8');
expect(html).toContain('cta-cell');
expect(html).toContain('Start a conversation');
});

it('has see-all projects link', () => {
const html = readFileSync(indexPath, 'utf8');
expect(html).toContain('bento-card--see-all');
});

it('does NOT have old ProjectGrid or home-pair', () => {
const html = readFileSync(indexPath, 'utf8');
expect(html).not.toContain('project-grid');
expect(html).not.toContain('home-pair');
expect(html).not.toContain('organ-group__toggle');
});

it('has dual-view data attributes', () => {
const html = readFileSync(indexPath, 'utf8');
expect(html).toContain('data-hero-view="engineering"');
expect(html).toContain('data-hero-view="creative"');
expect(html).toContain('data-stats-view="engineering"');
expect(html).toContain('data-stats-view="creative"');
Comment on lines +66 to +71
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Mar 27, 2026

Choose a reason for hiding this comment

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

issue (testing): Add a test to assert that bento cells do not use data-reveal, as mentioned in the PR description

The PR description specifies “No data-reveal on bento cells (above-fold, no scroll trigger needed)”, but there’s no test enforcing this. Please add a test that asserts data-reveal never appears on bento cell elements (or anywhere in the document, if that’s the requirement) to capture this behavior.

});
});
6 changes: 3 additions & 3 deletions src/data/quality-metrics.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
{
"generated": "2026-03-27T21:23:28.189Z",
"generated": "2026-03-27T21:41:24.133Z",
"tests": {
"total": 289,
"passed": 289,
"files": 34
"files": 35
},
"security": {
"critical": 0,
Expand Down Expand Up @@ -1704,7 +1704,7 @@
},
"build": {
"pages": 102,
"bundleFiles": 67
"bundleFiles": 66
},
"sources": {
"tests": ".quality/vitest-report.json",
Expand Down
4 changes: 2 additions & 2 deletions src/data/trust-vitals.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
"approval": "Student evaluation approval rating across all teaching assignments"
}
},
"generatedAt": "2026-03-27T21:23:28.448Z",
"fingerprint": "AB88D898",
"generatedAt": "2026-03-27T21:41:24.446Z",
"fingerprint": "85D36BA3",
"strikes": {
"total": 2,
"conversionRate": 0
Expand Down
9 changes: 9 additions & 0 deletions src/styles/global.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,15 @@ a.bento-cell:focus-visible {
margin-top: var(--space-md);
}

@media (max-width: 767px) {
.stat-pills {
flex-wrap: nowrap;
overflow-x: auto;
-webkit-overflow-scrolling: touch;
padding-bottom: var(--space-2xs);
}
}

.stat-pill {
display: inline-flex;
align-items: baseline;
Expand Down
Loading