Repository navigation
Add Messages design gallery and tighten message layout - #158
Conversation
Signed-off-by: Arjun Mahanti <[email protected]>
Signed-off-by: Arjun Mahanti <[email protected]>
Signed-off-by: Arjun Mahanti <[email protected]>
|
🤖 @wesbillman — please review the Messages design gallery and spacing pass. The PR includes 33 production-rendered specimens and 13 screenshots covering light/dark categories and narrow layouts: screenshot index. The changes cover author avatar sizing, avatar-to-message spacing, table corners/borders, thread-button padding and participant overlap, and membership spacing. Local hooks pass (2,218 unit tests plus types/design guards), and the built gallery passes in Chromium and WebKit. DCO passed; hosted CI is running. The PR description records the earlier app-page test timeout and deferred gallery coverage. |
Signed-off-by: Arjun Mahanti <[email protected]> # Conflicts: # tests/fixtures/design-system/viewer.spec.ts
Signed-off-by: Arjun Mahanti <[email protected]>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 gallery fullscreen integration defect, detailed inline. The P3 header-only table edge is non-blocking polish.
Reviewed d4ccd6bc6301f37676a828776dc9ed8974733a9b against base/merge base ee2051d000ce74eb20fb0c27df71d29efe792d37, including the update that arrived during review. The stale avatar assertion and earlier 38px artwork discrepancy are resolved; merge conflicts are also cleared.
Validation: design:build passed; the full avatar-shapes.spec.mjs passed Chromium/WebKit (2 tests). Focused built-gallery probes reproduced the fullscreen defect in both engines and confirmed the same viewer works when the child document is opened standalone. Broad CI is still in progress at this head; I did not rerun the full gallery suite, native, or live-relay workflows.
Merge criterion: make the advertised fullscreen interaction visible and pointer-reachable inside the gallery, retaining document isolation, with opening/Close/Escape/focus regression coverage in both engines. No production modal rewrite is needed.
| ref={frame} | ||
| title="Message types and states" | ||
| src={source} | ||
| style={{ width: "100%", height, border: 0, display: "block" }} |
There was a problem hiding this comment.
[P2] Full-height iframe puts video fullscreen outside the visible viewport
The child uses the production MediaAttachment fullscreen viewer, which portals to the child document.body and centers a fixed dialog against that document’s viewport. Setting the iframe height to the entire gallery (9,801px here) makes that viewport much taller than the visible browser window.
Reproduce from the built viewer at 1280×900: open Messages with the default All messages, scroll to Video attachment, and click Open video fullscreen. In Chromium the page dims but the dialog is entirely above the visible area (y=-1278.5, height 864; Close y=-1262.5); a normal pointer Close cannot reach it. Escape recovers. WebKit scrolls to show only part of the dialog (y=417.5, height 864). Opening the child gallery standalone works in both engines, so this is the new iframe integration, not a production-modal defect.
Keep the separate-document boundary, but give this interaction a viewport-sized presentation through a fixture-local solution. Add coverage that actually opens fullscreen from All messages and verifies visible playback, pointer Close, Escape and focus restoration in both engines. The new browser case checks decoding but never opens the viewer.
| .text :where(th, td):last-child { | ||
| border-right: 0; | ||
| } | ||
| .text tbody tr:last-child td { |
There was a problem hiding this comment.
[P3, non-blocking] Header-only tables retain a doubled bottom edge
Valid GFM containing only | A | B | and its delimiter row renders <table><thead>…</thead></table> with no tbody. This terminal-border rule therefore never removes the header cells’ bottom border, leaving it adjacent to the new outer bottom border. I verified 1px on both edges in Chromium/WebKit using the emitted Markdown DOM.
Also suppress the final header-row bottom edge when thead is the last row group (without removing the separator on tables with body rows), and cover that header-only case.
Signed-off-by: Arjun Mahanti <[email protected]>
Signed-off-by: Arjun Mahanti <[email protected]>
|
🤖 @wesbillman / Carl — CI-fix repush is ready at Since the reviewed Please take another look at the repush. This CI repair does not address Carl's separate gallery-fullscreen finding; that review request remains outstanding. |
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: the previously reported P2 gallery fullscreen defect remains, detailed inline. Reviewed 804413b22c3866af4b6710d99b154a0414e3b142 against base/merge-base 1f84835bbb7bbb83af8a4b45094ca3414ffa7a27.
Merge criterion: keep the gallery’s fullscreen content and Close control inside the visible viewport while retaining document isolation, with opening, pointer Close, Escape and focus-restoration regression coverage in both browser engines. The iframe, media viewer and relevant gallery test are unchanged from the previously reviewed head. The existing P3 header-only table edge remains non-blocking; no new finding is being added for it.
Validation: read-only Blox source review of gallery/build isolation, sizing/theme/reset lifecycle, production message consumers and toast hit testing, with an independent fullscreen review. No checkout, PR code execution, tests, builds or previews. Exact-head hosted CI passed its required JavaScript, Rust/tool, browser journey and measurement lanes; security and DCO checks passed. Windows native validation was skipped. This is not a new browser reproduction or native/live-relay acceptance.
| ref={frame} | ||
| title="Message types and states" | ||
| src={source} | ||
| style={{ width: "100%", height, border: 0, display: "block" }} |
There was a problem hiding this comment.
P2: Keep fullscreen media within the visible gallery viewport.
The iframe grows to the entire gallery content height (main.tsx:177–191), but the Video attachment specimen still invokes the production fullscreen fallback. MediaAttachment.tsx:262–285 portals into the child document; Messages.module.css:556–574 fixes and centers the viewer against that much taller iframe viewport, not the portion visible in the outer page. useModalBoundary.ts:22–35 inerts the gallery and focuses Close without correcting the cross-document geometry.
Open Product patterns → Messages, filter to Attachments and activate “Open video fullscreen”: the backdrop can cover the visible gallery while the video and pointer Close control sit outside the visible region. The responsible iframe/gallery/modal paths are unchanged from d4ccd6b, where this was reproduced in Chromium and WebKit. The new gallery test checks decoding and width, but never opens fullscreen (viewer.spec.ts:1165–1263), so green CI does not cover this interaction.
Constrain the gallery’s fullscreen presentation to the visible viewport without rewriting the production modal or breaking document isolation. Add a real iframe browser regression for opening, visible pointer-reachable content/Close, Escape, and focus restoration in both engines.
Signed-off-by: Arjun Mahanti <[email protected]>
Signed-off-by: Arjun Mahanti <[email protected]>
Signed-off-by: Arjun Mahanti <[email protected]>
|
🤖 @wesbillman / Carl — the blocking fullscreen finding is fixed; please re-review The gallery keeps its separate document but bounds it to the visible viewport, with gallery scrolling and a bounded navigation area on narrow screens. The production media viewer is unchanged. The new regression opens fullscreen from both All messages and Attachments at 1280/800/390px and checks full video/Close visibility, playback, pointer Close, Escape, and focus restoration in Chromium and WebKit. It failed before the fix; the complete viewer file now passes 54/54 locally, and the gallery checks also passed hosted CI at That hosted run found an unrelated Settings test focus-handoff race. The latest commit waits for the existing menu-to-page focus completion before typing; the full Settings and Appearance files pass 20/20 in both engines. The Appearance helper needed the same handoff barrier when its radio-focus assertion exposed the same race in the next hosted run. No retries, delays, weakened assertions, or production changes were added for it. Required hooks pass, including 2,622 unit tests and type/design checks; DCO passes. Fresh hosted CI passed across every automatic lane, including all four browser shards and CI required. The nonblocking header-only Markdown table P3 remains follow-up. Native packaged-app/live-relay acceptance remains deferred. PR description has the full validation scope. Not merged. |
Signed-off-by: Arjun Mahanti <[email protected]>
|
🤖 @wesbillman / Carl — final follow-up: |
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No blocking findings at 33bad9d975b856a953157bf3aea5b494b825bb06. The prior fullscreen P2 is addressed: the gallery now scrolls inside a viewport-bounded iframe, preserving the separate document/build boundary and reusing the production media viewer. The new regression checks full dialog/video/Close visibility, pointer Close, Escape, and focus restoration across three widths and both category states. The known header-only table-border P3 remains nonblocking.
Source-only review covered gallery lifecycle/build isolation, production message/avatar/table/thread styles, toast hit-testing, and the Settings/Appearance focus-handoff changes. Existing exact-head CI passed, including all four Chromium/WebKit journey shards and 2,622 unit tests. I did not execute PR code or rerun browsers; the author’s standalone viewer fail-before/pass-after results were not independently reproduced. Native packaged-app and live-relay acceptance remain unverified; Windows native validation was skipped.
Nonblocking follow-up: directly assert category exclusion and toast gap/card hit-testing; native scrollbar dragging on an overflowing pass-through toast viewport was not exercised. These are coverage gaps, not demonstrated blockers.
This is a clear review recorded as COMMENTED, not an approval.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
@mahanti do you want these reviews/message-polish screenshots in the project?
Signed-off-by: Arjun Mahanti <[email protected]>
Signed-off-by: Arjun Mahanti <[email protected]>
|
🤖 @wesbillman — removed all 13 The merged app/design builds pass, as do all 56 viewer and eight Settings browser checks in Chromium/WebKit. Updated main's stale rich-composer test fixtures without production changes; all mandatory type/unit/design hooks and DCO pass. Fresh hosted CI is running. Please re-review the refreshed head. |
Signed-off-by: Arjun Mahanti <[email protected]>
|
🤖 @wesbillman / Carl — please re-review Hosted CI is fully green, including all four Chromium/WebKit shards, measurements, JavaScript, Rust/tool integration, CI required, security, and DCO. Local hooks passed 2,786 unit tests; the full affected new-message file passes both engines. The prior fullscreen fix remains intact. Please clear the screenshot review request if satisfied; not merged. |
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No blocking findings at 5fe61b178328e368c7abc9dbe109718511bc17b8 against base/merge base 597c09719b51ca84ec58baa5c2d57f87dd7c40f6. This is a follow-up to the clear review at 33bad9d, focused on main integration, screenshot removal, and the new fixture repairs.
The merge retains both Forms and Messages routes/styles/tests. The gallery document/build isolation and repaired fullscreen sizing are unchanged. The composer/media test updates match the current production APIs while retaining behavioral assertions. docs/reviews/message-polish/, including its 13 screenshots and index, is absent from this head.
Validation: exact-commit source review with independent production-layout and gallery-integration lanes; git diff --check passes. Hosted CI is verified green at this head, including all four Chromium/WebKit app-journey shards, JavaScript, Rust/tools, measurements, security and DCO. Windows native validation is skipped. No PR code, builds, tests, or browsers were executed locally in this review. The reported standalone viewer 56/56 result remains author-provided evidence, not an independently reproduced result; native packaged-app/live-relay acceptance remains unverified.
The existing header-only table-border P3 remains nonblocking. No new merge criteria. This is a clear review recorded as COMMENTED, not an approval.
…rs-support * origin/main: Add Messages design gallery and tighten message layout (#158) Fix diff content fallback, keyboard scrolling and edit selection (#205) Standardize form controls and field feedback across Buzz (#174) Keep image review downloads and external opens distinct (#144) Verify media review comments (#166) Follow system appearance (#210) Add rich composer formatting and spoiler rendering (#203) feat: show roster-backed channels and managed instances in profiles (#188) Add new direct message flow (#156) Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
…o-player-polish * origin/main: Add Messages design gallery and tighten message layout (#158) Signed-off-by: Zach Marley <[email protected]>
…-content-compat * origin/main: Add Messages design gallery and tighten message layout (#158) Signed-off-by: Zach Marley <[email protected]> # Conflicts: # src/features/direct-messages/NewMessage.recovery.test.tsx
Messages lacked a shared visual inventory and used inconsistent row spacing. This adds Product patterns → Messages to
just design, rendered by production components, and applies the reviewed message layout changes.All 13 review screenshots and their index were removed. Main at
597c0971is integrated without conflicts, including both Forms and Messages viewer coverage. Media/reply and rich-composer test fixtures now match current contracts. Appearance waits for the completed Settings focus handoff; the new-message journey checks the placeholder on the rich editor’s paragraph.Validation
Hosted CI is fully green at
5fe61b178328e368c7abc9dbe109718511bc17b8: all four Chromium/WebKit shards, browser measurements, JavaScript, Rust/tool integration, CI required, security, and DCO. Required local hooks pass, including 2,786 unit tests and type/design guards. Every PR commit has a sign-off.Local app/design builds pass. After main integration, the complete viewer file passed 56/56 checks and Settings passed 8/8 across Chromium and WebKit. The final placeholder correction passes the complete new-message file in both engines (2/2). Earlier targeted profile/avatar and toast checks also passed both engines.
Browser coverage and remaining checks
Two gallery browser cases added, none removed. They cover isolated asset loading, theme propagation, filters/interactions, overflow, and native media behavior. The fullscreen regression failed in both engines before the fix and passes All messages/Attachments at 1280px, 800px, and 390px, including decoded playback, pointer Close, Escape, and focus restoration. Cross-document viewport geometry, hit testing, and media playback require a real browser. Existing avatar paint and focus-handoff assertions are retained; no retries, sleeps, timeout increases, or forced clicks were introduced.
Reviewer approval and native packaged-app/live-relay acceptance remain outstanding. The known header-only table-border P3 is nonblocking follow-up. Audio/voice notes, composer, presence, unread, and history-loading gallery examples remain future coverage.