[WOOMOB-3561] Add collapsible Page Header behavior#16257
Conversation
Generated by 🚫 Danger |
|
|
There was a problem hiding this comment.
AI Code Review - No issues found. The changes look good.
The WooPageHeader rewrite to Material3 TopAppBar/MediumTopAppBar is clean and well-scoped. Nice touches: the WooPageHeaderScrollBehavior wrapper hides the experimental TopAppBarScrollBehavior behind an internal delegate and exposes only the stable NestedScrollConnection, and the new scrollBehavior param is added before the trailing actions lambda so existing callers stay source-compatible. The component is only consumed inside the design-system module, so the internal layout change has no production regression risk. Test coverage is thorough — fixed vs. medium heights, partial/collapsed states, single-heading accessibility, long-title truncation before the action, and LTR/RTL logical edges.
Automatic review · claude-opus-4-8 · Workflow run
How to reply to a finding
Reply on this review (or inline at the line the finding refers to) with one of:
@claude addressed- I made the change. Bot verifies against the next diff before marking resolved.@claude rejected: <reason>- Will not fix; reason gets quoted on the next review.@claude not-applicable- Finding does not apply (wrong file, already covered elsewhere, etc.).
The bot honours these on the next review pass.
c700fe6 to
ae30b7a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #16257 +/- ##
============================================
- Coverage 41.15% 41.12% -0.04%
- Complexity 13498 13503 +5
============================================
Files 2503 2504 +1
Lines 146462 146658 +196
Branches 21761 21817 +56
============================================
+ Hits 60275 60311 +36
- Misses 80089 80241 +152
- Partials 6098 6106 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@claude review |
There was a problem hiding this comment.
AI Code Review - Follow-up
The previous review raised no issues, so there is nothing to reconcile.
- Previous review readiness: not flagged (design-system component, no production consumers) — still resolved
New issues: 0
I re-reviewed the full current diff. The concurrency design in WooPageHeaderScrollBehavior.expand() holds up: the LAZY child job + writeIfActiveOwner ownership check correctly rejects stale writes when a newer expansion supersedes an older one, superseded callers return normally (individually cancelling a child does not propagate to the caller's coroutineScope), and the WooPageHeaderNestedScrollConnection cancels the active expansion before delegating meaningful user input and both fling boundaries. All of this is precisely exercised by the two test files. WooPageHeader remains consumed only inside the design-system module (component + catalog preview), so the API change carries no production regression risk, and the new implementation dependencies (compose-animation, coroutines-core) match the new code. Looks good.
Automatic review (follow-up) · claude-opus-4-8 · Workflow run
How to reply to a finding
Reply on this review (or inline at the line the finding refers to) with one of:
@claude addressed- I made the change. Bot verifies against the next diff before marking resolved.@claude rejected: <reason>- Will not fix; reason gets quoted on the next review.@claude not-applicable- Finding does not apply (wrong file, already covered elsewhere, etc.).
The bot honours these on the next review pass.
There was a problem hiding this comment.
AI Code Review - Follow-up
The previous review raised no code issues, so there is nothing to reconcile beyond the readiness note.
- Previous review readiness: not flagged (design-system component with no production consumers; exercised via catalog previews and unit tests) — still resolved
New issues: 0
The HEAD commit is unchanged from the previous follow-up, so there are no new changes to review. The WooPageHeaderScrollBehavior.expand() concurrency design remains sound: the LAZY child job plus the writeIfActiveOwner ownership guard rejects stale writes when a newer expansion supersedes an older one, superseded callers return normally, and WooPageHeaderNestedScrollConnection cancels the active expansion before delegating meaningful user input and both fling boundaries — all covered by the two test files. WooPageHeader is still consumed only inside the design-system module, so the API change carries no production regression risk, and the added implementation dependencies (compose-animation, coroutines-core) match the new code. Looks good.
Automatic review (follow-up) · claude-opus-4-8 · Workflow run
How to reply to a finding
Reply on this review (or inline at the line the finding refers to) with one of:
@claude addressed- I made the change. Bot verifies against the next diff before marking resolved.@claude rejected: <reason>- Will not fix; reason gets quoted on the next review.@claude not-applicable- Finding does not apply (wrong file, already covered elsewhere, etc.).
The bot honours these on the next review pass.
bf6658c to
616250d
Compare
|
Note: Wagner approved the collapsing animation of the title, and that it should match the default M3. |
irfano
left a comment
There was a problem hiding this comment.
I left some minor feedback, but I'm approving. 👍🏻

Description
Fixes WOOMOB-3561
WooPageHeaderremains one component API with two rendering modes:scrollBehavior = null, it renders the existing fixed 64dpTopAppBar.WooPageHeaderScrollBehavior, it renders Material 3'sMediumTopAppBarin its standard expanded, partial, and collapsed states.The default Material 3
MediumTopAppBarcollapsing behavior is deliberately preserved. Its expanded and collapsed title treatments cross-fade; the title does not continuously resize during collapse, and trailing actions remain pinned.WooPageHeaderDefaults.exitUntilCollapsedScrollBehavior()disables snapping and exposes thenestedScrollConnectionthat hosts attach to their scrolling container.WooPageHeaderScrollBehavioralso exposes suspendingexpand()for downstream programmatic expansion without exposingTopAppBarStateor experimental Material types.Material 3 remains responsible for ordinary nested scrolling, direct header drag, and decay settling. Woo only owns the active programmatic expansion and cancels it before delegating meaningful nested user input or fling boundaries. Concurrent
expand()calls follow a newest-wins policy: the superseded call returns normally, the newest invocation owns the final state, and the caller's ambient coroutine is not canceled.A deliberate non-guarantee remains for rare overlap: direct header drag or an already-running private Material 3 settle can race with programmatic expansion. Material keeps the state clamped to valid bounds, but a later interaction or
expand()may be needed to correct the visual state.Scope is limited to the design-system component, catalog/demo, documentation, and focused tests. Dashboard is intentionally not migrated and no
WooCommerce/app integration is included. The catalog now includes an interactive collapse sample, while focused unit and Robolectric coverage exercises geometry, title/semantics behavior, RTL, scrolling, direct drag/fling, programmatic expansion, and cancellation boundaries.No release note is added because this is an internal design-system component/API change with no app integration.
Test Steps
Images/gif
Collapse behavior:
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.