feat: content-aware column widths, width estimation tests, and screenshot comparison infra#606
Merged
feat: content-aware column widths, width estimation tests, and screenshot comparison infra#606
Conversation
- 16 Storybook stories reproducing #587, #595-#597, #599-#602 - Playwright spec capturing screenshots with SCREENSHOT_DIR env var - CI job StylingScreenshots: captures after (current) + before (b7956f8) via git worktree, uploads as two named artifacts - download_styling_screenshots.sh: fetch artifacts via gh CLI - gen_screenshot_compare.py: self-contained before/after HTML viewer Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📦 TestPyPI package publishedpip install --index-strategy unsafe-best-match --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ buckaroo==0.12.12.dev22511604837or with uv: uv pip install --index-strategy unsafe-best-match --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ buckaroo==0.12.12.dev22511604837MCP server for Claude Codeclaude mcp add buckaroo-table -- uvx --from "buckaroo[mcp]==0.12.12.dev22511604837" --index-strategy unsafe-best-match --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ buckaroo-table |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 921c5a3991
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| const STORIES = [ | ||
| // Section A – width/contention (#595, #596, #599, #600) | ||
| { id: 'buckaroo-dfviewer-stylingissues--fewcols-shorthdr-shortdata', name: 'A1_FewCols_ShortHdr_ShortData', issues: '#599' }, |
There was a problem hiding this comment.
The id values in this spec don't match Storybook's generated slugs for these exports (e.g. FewCols_ShortHdr_ShortData resolves to ...--few-cols-short-hdr-short-data, not ...--fewcols-shorthdr-shortdata), so the Playwright run navigates to missing-story pages instead of the target stories. Because the wait condition accepts #storybook-root, these tests can still pass and produce screenshots of error/fallback content, which makes the before/after artifacts misleading rather than validating the styling regressions.
Useful? React with 👍 / 👎.
- Fix Storybook story IDs: export names go through startCase() before slugifying, so FewCols_ShortHdr_ShortData → few-cols-short-hdr-short-data - Remove extraneous f-string prefixes in gen_screenshot_compare.py (ruff F541) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The reuseExistingServer:true in playwright config caused the "before" screenshots to be captured against the still-running "after" storybook, making all screenshots identical. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Left sidebar (20%) with story list and keyboard navigation (j/k/arrows). Detail view with stacked before/after images, zoom slider (50-500%), and pan X/Y controls that apply to both images simultaneously. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This reverts commit 4047c0c.
Remove the broken worktree-based before/after approach. Each branch just captures its own screenshots and uploads one artifact. Compare by running CI on two separate branches. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
GitHub PR events checkout a merge of PR+base, so the baseline branch was getting main's fixes merged in. Use head.sha to get the actual branch code. Also use --no-frozen-lockfile for older branches. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The AG-Grid is inside a Shadow DOM wrapper, so page.locator can't find .ag-body-viewport. Use page.evaluate to access shadowRoot. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…croll C13/C14/D15 had short headers that fit in 800px with no scrollbar. Use long headers + scroll to scrollWidth to expose #587 alignment bug. Also simplified scroll code — Playwright locators pierce shadow DOM. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fitCellContents was shrinking all columns to fit 800px viewport (scrollWidth == clientWidth). Narrower container forces overflow so the scroll-right in the spec actually works. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…erflow fitCellContents shrinks all columns to fit container regardless of defaultMinWidth. Explicit minWidth on ColDef forces horizontal scroll. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AG-Grid fitCellContents prevents programmatic scroll (scrollWidth==clientWidth). Click a cell then press End to let AG-Grid handle navigation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
15 cols with long headers (revenue_total, etc.) and 4-digit year values. Without defaultMinWidth, fitCellContents crushes columns to data width (~35px), truncating headers. With the fix, columns get min 80px. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fitCellContents never crushes columns, so use ag_grid_specs maxWidth to cap columns at 50px. 7-digit data should show "..." truncation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Set minWidth:80 on defaultColDef so AG-Grid enforces a minimum column width regardless of auto-size strategy - Remove broken defaultMinWidth on fitCellContents (that property only exists on fitGridWidth strategy and was silently ignored) - Update A9 story to demonstrate the fix: width:40 + suppressAutoSize creates narrow columns; minWidth:80 overrides to show readable values Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Navigate directly to a story via URL hash (e.g. compare.html#A9_ManyCols_LongHdr_YearData). Also update A9 tag from no-diff to diff. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…trategy override - A9 now uses fitGridWidth strategy (15 cols in 800px → ~53px each) - extra_grid_config.autoSizeStrategy takes precedence over default - minWidth:80 on defaultColDef prevents columns from crushing → readable values Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
25 cols in 800px → ~32px each without minWidth → "2,..." on every cell. With minWidth:80 → 10 readable columns with horizontal scroll. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update StylingScreenshots CI job to: - Start Storybook (was missing, causing Playwright failures) - Checkout baseline b7956f8, copy stories, capture "before" screenshots - Checkout current branch, capture "after" screenshots - Upload as two separate artifacts (styling-screenshots-before/after) The download script and compare viewer already expect this artifact layout. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pnpm install --no-frozen-lockfile at baseline modifies pnpm-lock.yaml, causing git checkout to fail. Use -f flag and git clean to discard changes from the baseline install. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace blanket defaultColDef.minWidth:80 with per-column widths computed from displayer type, data range, and header name length. Python side (styling.py): - estimate_min_width_px() computes pixel width from formatted char count - Accounts for commas in integers, decimal+fraction in floats, header length - Histogram pinned rows request 100px minimum - Set via ag_grid_specs.minWidth per column JS side: - Remove hardcoded minWidth:80 from defaultColDef - Per-column ag_grid_specs.minWidth already flows through via gridUtils Narrow columns (1-2 digit data, short headers) stay tight instead of being forced to 80px. Wide columns get appropriate minimums. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…istogram stories - 23 unit tests for estimate_min_width_px() and _formatted_char_count() covering all displayer types, edge cases (NaN, None, negative), and histogram minimum width enforcement - 3 new stories (Section E) exercising Python-computed ag_grid_specs.minWidth with realistic values (38px narrow ints, 134px long headers, mixed types) - 2 new stories (Section F) showing histogram + narrow columns: one with 100px histogram minimum, one without (to show the difference) - Updated Playwright spec and compare tool for new stories - Added docs/column-width-design.md documenting all width levers and approaches Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The histogram renderer expects [{name, population}] objects, not raw
number arrays. Fixes blank histogram rows in F20/F21 stories.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
paddymul
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
minWidth: Python'sestimate_min_width_px()computes pixel width from displayer type, data range, header name, and histogram presence — replaces the blanketdefaultColDef.minWidth: 80that wasted space on narrow columnsdocs/column-width-design.mddocuments all width levers (per-columnag_grid_specs, grid-levelautoSizeStrategy, displayer choice, theme params) and four valid sizing approaches (readable, compact, balanced, data-priority)Key changes
Python (
buckaroo/customizations/styling.py)_formatted_char_count()estimates character width per displayer type (float, integer, compact_number, string, datetime)estimate_min_width_px()computes pixel width from max(data width, header width), with 100px histogram floorDefaultMainStyling.style_column()setsag_grid_specs.minWidthper columnJS (
DFViewerInfinite.tsx,gridUtils.ts)minWidth: 80fromdefaultColDefag_grid_specsflows through...f.ag_grid_specsspread inbaseColToColDef()extra_grid_config.autoSizeStrategyoverride supportedStories (22 total across 6 sections)
ag_grid_specs.minWidth(3 stories — narrow ints, long headers, mixed types)CI (
.github/workflows/checks.yml)StylingScreenshotsjob captures before screenshots at baselineb7956f8, after at current branchTest plan
estimate_min_width_px()and_formatted_char_count()pass🤖 Generated with Claude Code