Skip to content

Commit 7b6a05c

Browse files
paddymulclaude
andauthored
feat: content-aware column widths, width estimation tests, and screenshot comparison infra (#606)
* feat: add StylingIssues stories and before/after screenshot CI - 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> * fix: correct story IDs in spec + remove spurious f-string prefixes - 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> * fix: kill storybook between after/before screenshot captures 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> * feat: redesign screenshot compare viewer with sidebar nav and zoom/pan 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> * Revert "fix: kill storybook between after/before screenshot captures" This reverts commit 4047c0c. * simplify StylingScreenshots CI job — single artifact per branch 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> * fix: checkout PR head SHA for screenshots, not merge ref 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> * fix: scroll grid body in pinned-index screenshots to expose #587 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: pierce shadow DOM for pinned-index scroll in screenshots 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> * fix: walk all shadow roots to find ag-body-viewport for scroll Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use long headers in pinned stories so grid requires horizontal scroll 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> * debug: add scroll logging for pinned-index screenshots Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use 400px container for pinned stories to force horizontal scroll 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> * fix: add minWidth:120 via ag_grid_specs on pinned stories to force overflow 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> * fix: use keyboard End key to scroll pinned stories to rightmost column 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> * feat: add A9 story for #595 — many cols, long headers, year-like data 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> * fix: A9 story — 25 long cols in 400px to force data truncation (#595) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: A9 — force maxWidth:50 on columns to reproduce #595 truncation 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> * fix: add defaultColDef.minWidth:80 to prevent column truncation (#595) - 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> * feat: add hash routing to screenshot compare viewer 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> * fix: use fitGridWidth for A9 story; allow extra_grid_config.autoSizeStrategy 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> * fix: use 25 cols in A9 story to force visible truncation at fitGridWidth 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> * ci: capture before/after styling screenshots from baseline b7956f8 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> * fix: force-checkout current branch after baseline pnpm install 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> * feat: content-aware per-column minWidth from Python analysis 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> * feat: add width estimation tests, Python-computed minWidth stories, histogram 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> * fix: use HistogramBar[] format for histogram story summary data 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> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent fcfe368 commit 7b6a05c

13 files changed

Lines changed: 1998 additions & 15 deletions

File tree

.github/workflows/checks.yml

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -407,6 +407,106 @@ jobs:
407407
path: packages/buckaroo-js-core/screenshots/
408408
if-no-files-found: ignore
409409

410+
StylingScreenshots:
411+
name: Styling Screenshots (Before / After)
412+
runs-on: depot-ubuntu-latest
413+
timeout-minutes: 25
414+
steps:
415+
- uses: actions/checkout@v6
416+
with:
417+
ref: ${{ github.event.pull_request.head.sha || github.sha }}
418+
fetch-depth: 0
419+
- name: Setup pnpm
420+
uses: pnpm/action-setup@v4
421+
with:
422+
version: 9.10.0
423+
- name: Setup Node.js with pnpm cache
424+
uses: actions/setup-node@v6
425+
with:
426+
cache: 'pnpm'
427+
cache-dependency-path: 'packages/pnpm-lock.yaml'
428+
- name: Cache Playwright browsers
429+
uses: actions/cache@v5
430+
with:
431+
path: ~/.cache/ms-playwright
432+
key: playwright-${{ hashFiles('packages/buckaroo-js-core/package.json') }}
433+
434+
# Save story + spec from current branch (baseline commit won't have them)
435+
- name: Save story and spec files
436+
run: |
437+
mkdir -p /tmp/styling-stories
438+
cp packages/buckaroo-js-core/src/stories/StylingIssues.stories.tsx /tmp/styling-stories/
439+
cp packages/buckaroo-js-core/pw-tests/styling-issues-screenshots.spec.ts /tmp/styling-stories/
440+
441+
# ── Before screenshots (baseline b7956f8, pre-#587) ──
442+
- name: Checkout baseline (b7956f8)
443+
run: git checkout b7956f8
444+
- name: Copy stories to baseline
445+
run: |
446+
cp /tmp/styling-stories/StylingIssues.stories.tsx packages/buckaroo-js-core/src/stories/
447+
cp /tmp/styling-stories/styling-issues-screenshots.spec.ts packages/buckaroo-js-core/pw-tests/
448+
- name: Install pnpm deps (baseline)
449+
working-directory: packages
450+
run: pnpm install --no-frozen-lockfile
451+
- name: Install Playwright browsers
452+
working-directory: packages/buckaroo-js-core
453+
run: pnpm exec playwright install chromium
454+
- name: Capture before screenshots
455+
working-directory: packages/buckaroo-js-core
456+
env:
457+
SCREENSHOT_DIR: screenshots/before
458+
run: |
459+
pnpm storybook --no-open &
460+
SB_PID=$!
461+
timeout=60; elapsed=0
462+
while ! curl -sf http://localhost:6006 > /dev/null 2>&1; do
463+
sleep 2; elapsed=$((elapsed + 2))
464+
if [ $elapsed -ge $timeout ]; then echo "Storybook failed to start"; exit 1; fi
465+
done
466+
pnpm exec playwright test pw-tests/styling-issues-screenshots.spec.ts --reporter=line || true
467+
kill $SB_PID 2>/dev/null || true
468+
lsof -ti:6006 | xargs kill -9 2>/dev/null || true
469+
470+
# ── After screenshots (current branch) ──
471+
- name: Checkout current branch
472+
run: |
473+
git checkout -f ${{ github.event.pull_request.head.sha || github.sha }}
474+
git clean -fd packages/
475+
- name: Install pnpm deps (current)
476+
working-directory: packages
477+
run: pnpm install --no-frozen-lockfile
478+
- name: Capture after screenshots
479+
working-directory: packages/buckaroo-js-core
480+
env:
481+
SCREENSHOT_DIR: screenshots/after
482+
run: |
483+
pnpm storybook --no-open &
484+
SB_PID=$!
485+
timeout=60; elapsed=0
486+
while ! curl -sf http://localhost:6006 > /dev/null 2>&1; do
487+
sleep 2; elapsed=$((elapsed + 2))
488+
if [ $elapsed -ge $timeout ]; then echo "Storybook failed to start"; exit 1; fi
489+
done
490+
pnpm exec playwright test pw-tests/styling-issues-screenshots.spec.ts --reporter=line
491+
kill $SB_PID 2>/dev/null || true
492+
lsof -ti:6006 | xargs kill -9 2>/dev/null || true
493+
494+
# ── Upload artifacts ──
495+
- name: Upload before screenshots
496+
if: always()
497+
uses: actions/upload-artifact@v4
498+
with:
499+
name: styling-screenshots-before
500+
path: packages/buckaroo-js-core/screenshots/before/
501+
if-no-files-found: ignore
502+
- name: Upload after screenshots
503+
if: always()
504+
uses: actions/upload-artifact@v4
505+
with:
506+
name: styling-screenshots-after
507+
path: packages/buckaroo-js-core/screenshots/after/
508+
if-no-files-found: ignore
509+
410510
TestServer:
411511
name: Server Playwright Tests
412512
needs: [BuildWheel]

buckaroo/customizations/styling.py

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,61 @@
22
from typing import Any
33
from buckaroo.styling_helpers import obj_, float_, inherit_, pinned_histogram
44

5+
# Pixel-width estimation constants, calibrated to AG-Grid theme with
6+
# spacing:5, cellHorizontalPaddingScale:0.3, fontSize:12, headerFontSize:14
7+
_CHAR_PX_DATA = 7 # approx width per character in data cells
8+
_CHAR_PX_HEADER = 8 # approx width per character in header
9+
_CELL_PAD = 16 # total horizontal padding inside a cell
10+
_SORT_ICON = 14 # sort indicator + gap in header
11+
_HISTOGRAM_MIN_PX = 100 # minimum width for a histogram pinned row to render usefully
12+
_MIN_COL_PX = 30 # absolute floor
13+
14+
15+
def _formatted_char_count(displayer_args, column_metadata):
16+
"""Estimate the character count of the widest formatted value."""
17+
d = displayer_args.get('displayer')
18+
19+
if d == 'float':
20+
max_val = column_metadata.get('max', 0) or 0
21+
min_val = column_metadata.get('min', 0) or 0
22+
max_abs = max(abs(max_val), abs(min_val))
23+
int_digits = max(1, len(str(int(max_abs)))) if max_abs == max_abs else 1 # nan guard
24+
commas = (int_digits - 1) // 3
25+
frac = displayer_args.get('max_fraction_digits', 0)
26+
decimal = 1 if frac > 0 else 0
27+
sign = 1 if min_val < 0 else 0
28+
return int_digits + commas + decimal + frac + sign
29+
30+
if d == 'integer':
31+
max_digits = displayer_args.get('max_digits', 4)
32+
commas = (max_digits - 1) // 3
33+
return max_digits + commas
34+
35+
if d == 'compact_number':
36+
return 5 # e.g. "5.7B"
37+
38+
if d == 'string':
39+
return min(displayer_args.get('max_length', 20), 20)
40+
41+
if d in ('datetimeLocaleString', 'datetimeDefault'):
42+
return 18 # "12/31/2024, 11:59 PM"
43+
44+
return 8 # obj / fallback
45+
46+
47+
def estimate_min_width_px(displayer_args, header_name, column_metadata, has_histogram=False):
48+
"""Compute a per-column minWidth in pixels from content analysis."""
49+
data_chars = _formatted_char_count(displayer_args, column_metadata)
50+
data_px = data_chars * _CHAR_PX_DATA + _CELL_PAD
51+
52+
hdr_chars = len(str(header_name)) if header_name else 1
53+
header_px = hdr_chars * _CHAR_PX_HEADER + _SORT_ICON + _CELL_PAD
54+
55+
width = max(data_px, header_px)
56+
if has_histogram:
57+
width = max(width, _HISTOGRAM_MIN_PX)
58+
return max(width, _MIN_COL_PX)
59+
560

661
class DefaultMainStyling(StylingAnalysis):
762
requires_summary = ["histogram", "is_numeric", "dtype", "_type"]
@@ -30,7 +85,17 @@ def style_column(kls, col:str, column_metadata: Any) -> Any:
3085
else:
3186
disp = {'displayer': 'obj'}
3287
base_config['tooltip_config'] = {'tooltip_type':'simple', 'val_column': str(col)}
33-
base_config['displayer_args'] = disp
88+
base_config['displayer_args'] = disp
89+
90+
# Compute content-aware minWidth
91+
header_name = column_metadata.get('orig_col_name', col)
92+
has_histogram = any(
93+
pr.get('displayer_args', {}).get('displayer') == 'histogram'
94+
for pr in kls.pinned_rows
95+
)
96+
min_w = estimate_min_width_px(disp, header_name, column_metadata, has_histogram)
97+
base_config['ag_grid_specs'] = {'minWidth': min_w}
98+
3499
return base_config
35100

36101

0 commit comments

Comments
 (0)