Repository navigation
Premium Analytics: use SectionHeader on the post and video detail pages - #51875
Merged
Merged
Conversation
Contributor
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
Bot
added
[Package] Premium Analytics
[Plugin] Jetpack
Contributor
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Premium Analytics plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 4 files.
7 files are newly checked for coverage. Only the first 5 are listed here.
|
The detail pages hand-rolled a third header beside the dashboard's and the report pages': a flex row with its own wrap rules, plus a 440px constant reserved for the date panel and a ref feeding it `containerElement`. SectionHeader gains the two parameters those pages needed and nothing else had — a `visual` slot and a `subTitle` — plus a `headingLevel`, since a detail page's title is its `h1`. Naming and the decorative contract on the visual follow `@wordpress/admin-ui`'s own page header, which the same files already render. The slot owns the visual's box, so both summary cards stop restating the 72px square and its radius. That also settles a latent difference between them: the post thumbnail hard-coded `8px` where the video poster read `--wpds-border-radius-lg`, which is 8px in the default theme and 0 in the zero-radius one. The pages keep the panel's responsive step-down through the header's container query, so the external measurement retires with the migration. On video detail, a failed or missing video now leaves the header with its date controls alone and states the reason below it, where the widgets would have been: an error is not something the shared header should know about. WOOA7S-1949
chihsuan
force-pushed
the
change/wooa7s-1949-section-header-detail-pages
branch
from
September 2, 2026 08:00
c0e74e7 to
e276e4d
Compare
From review of the SectionHeader migration.
* `title` goes back to a required prop. Optional, it let a page render a
header with no heading at all, which is what the video stage did in its
loading, error and not-found states: it spread `{}` into the header and
the page lost its `h1`. Required, the spread is a type error.
* `videoHeaderSlots` now owns every summary state, so the video page keeps
its `h1`, its visual box and its date controls throughout: a skeleton
while the summary resolves (matching the post page, WOOA7S-2059), and a
named state when it fails. The notice below the header is unchanged.
* `postHeaderSlots` names an untitled or failed post rather than dropping
the heading, and restores the `aria-busy` the summary card carried.
* Both slot factories and `performanceSentence` take the package's own
`DateRange` instead of three inline spellings of it, two of which the
real type could not be assigned to. `HeaderSlots` is now one exported
`Pick` of the header's props rather than a hand-copy in each file.
* `.withVisual` no longer widens the gap between the title and the
controls, so the stacking breakpoint constant matches the width the row
actually needs.
* Cover the untitled and failed paths, the video slots (which had no test
of their own), the date helpers' guards, and the stage wiring the mocks
were hiding; run the header's date helpers under test-tz.
…ection-header-detail-pages # Conflicts: # projects/packages/premium-analytics/routes/post-detail/stage.tsx # projects/packages/premium-analytics/routes/video-detail/stage.tsx
chihsuan
marked this pull request as ready for review
September 2, 2026 08:47
chihsuan
added
[Status] Needs Review
The fixture publishes at 08:00, so neither timezone test-tz runs shifts its day. The entry would suggest coverage it does not give.
Nikschavan
previously approved these changes
Sep 2, 2026
Nikschavan
left a comment
Nikschavan
left a comment
Member
There was a problem hiding this comment.
Thank you, the changes look good. I left three non-blocking notes inline.
| .title { | ||
| overflow: visible; | ||
| text-overflow: clip; | ||
| white-space: normal; |
Member
There was a problem hiding this comment.
The stacked layout relaxes white-space on .title only, so at a phone width the subtitle still runs on one line and the performance window is the part that gets cut: at 390px the line measures 435px in a 250px cell and ends at "Performan…". Letting .subTitle wrap inside section-header-stacked alongside .title would keep the window readable where the title already takes several lines.
| // bottom. A floor, not a height — the stacked layout lets the title wrap, | ||
| // and a fixed box would push the subtitle out from under it. | ||
| min-block-size: $section-header-visual-size; | ||
| justify-content: space-between; |
Member
There was a problem hiding this comment.
.withVisual .text keeps justify-content: space-between when the header has no subtitle, and with one flex child that puts the h1 at the top of the 72px cell: on the video not-found state the title centre sits at 157px against the icon centre at 173px, with the bottom half of the cell empty. justify-content: center plus margin-block-start: auto on .subTitle gives the same top/bottom split when both lines are present and centres a lone title.
Member
There was a problem hiding this comment.
The video not-found state at 1280px, this branch at 11317a3:
|
|
||
| describe( 'formatPublishedDate', () => { | ||
| it( 'reads an offset-less value as site wall time', () => { | ||
| expect( formatPublishedDate( '2026-01-10T08:00:00' ) ).toBe( 'Jan 10, 2026' ); |
Member
There was a problem hiding this comment.
This case cannot fail without toLocalTZ: the test script pins TZ=UTC, siteTimeZone() resolves to +00:00 under the default @wordpress/date settings, and a fixture at 08:00 lands on the same day in America/Los_Angeles and Asia/Tokyo as well, which is what the commit dropping routes/detail-header from test-tz observes. Pinning the site timezone to Asia/Tokyo with setSettings, as stats-queries.test.ts does for UTC, makes the same 08:00 fixture cross midnight in UTC, so the assertion turns on the conversion. Fine as a follow-up.
…etail stages (#51877) * refactor: share one page layout between the post and video detail stages Both stages carried their own copy of the same scaffold: a `.page`, a `.scrollArea`, a `.content` band and a `.widgets` block that was byte-identical down to its TODO. `DetailPageShell` / `DetailPageLayout` / `DetailPageSection` now own that structure, following `ReportPageShell` / `ReportPageLayout` next door, so the stages hand over their header slots, their date controls and their widget content and nothing else. The layout lives in `widgets-toolkit` beside `report-page` rather than under `routes/`: wp-build treats every directory in `routes/` as a route to build, so a shared folder there would be discovered as one. `.widgets` folds into the section: `--wp-grid-gap` is inherited and the Card rules are descendant selectors, so moving them one level out matches the same elements without an extra wrapper around the grid. Two differences the copies had drifted into, both invisible in the default theme, are settled. The header's `padding-block` converges on 40px, written as `--wpds-dimension-gap-3xl`, which the video page's comment records as the design's breathing room. Post detail reaches the same 40px only once its tab bar stops adding a bottom margin on top of it: #50540 zeroed that margin for exactly this reason and #50562 restored it for a two-row header that no longer exists, so 16px + 32px becomes 40px and the two pages finally agree. And the shared `.content` end padding was a bare 24px restating `--wpds-dimension-padding-2xl`. `postHeaderSlots` and `videoHeaderSlots` each declared their own copy of the slot shape; both now return `DetailPageHeaderSlots`, which the layout derives from `SectionHeaderProps` so a required `title` and `busy` stay required. `DetailPageTabs` re-exports the tab root beside the panel, so a detail route reaches both through one package specifier — the constraint `report-page-tabs` already documents. `DetailPageTabPanelProps` is declared and exported with its `TabId` generic rather than leaving consumers to import the underlying UI type. Drops the `display: flex` widget-chrome workaround: `@wordpress/widget-dashboard` 0.5.0, the version pinned here, ships the upstream fix (WordPress/gutenberg#80570) in `.widget-chrome` itself. The suites assert the classes the gutters, the grid gap and the Card padding hang off, with a local style mock — which keeps them out of the shared no-mocks group, per `tests/groups/README.md`. WOOA7S-1949 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W4VNbKvETw12J6L1nCzixU * change: settle the three review notes on the shared detail page layout The post tab bar's `margin-block-end: 0` lands on the same element as `SectionTabs`' own rule, both single-class, so it only won because the toolkit's stylesheet is emitted first. Doubling the selector makes it win outright, and an import reorder can no longer move the header spacing. `DetailPageTabs` and `DetailPageTabPanel` move into `detail-page-tabs.tsx`, matching `report-page-tabs` next door. The layout no longer imports `SectionTabPanel`, so `video-detail/stage.test.tsx` drops the mock it carried for a page with no tabs — a mock the suite already passed without. `DetailPageShell`'s test mock swallowed `...pageProps`, leaving the one thing the component does beyond `clsx` unasserted. The mock now captures the rest props and the test checks `visual` and `breadcrumbs` reach `Page`; removing the spread fails it. WOOA7S-1949 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LouhrnbjGjdAK2BZqC2M7s --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
[Status] UI Changes
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
2 participants
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.
Fixes WOOA7S-1949
Proposed changes
SectionHeaderon the Premium Analytics post and video detail pages, replacing the separate custom headers.SectionHeaderwith a visual, a subtitle, and a heading level so detail pages can use it as the pageh1while existing dashboard and report headers continue to work as before.SectionHeadermanage the responsive layout for the title and date controls. This removes the detail pages' separate width reservation and container wiring.The API and naming for the new visual slot follow the
@wordpress/admin-uipage-header pattern already used by these pages.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
/wp-admin/admin.php?page=jetpack-premium-analytics-wp-admin#/post/<id>.h1title, its publish date/performance range, and the date controls./wp-admin/admin.php?page=jetpack-premium-analytics-wp-admin#/video/<id>and confirm the video poster or fallback icon appears with the same header layout.Back to VideosorRetry) below it.The detail-page breakpoint is 812px (
720px+72pxvisual +20pxgap). The dashboard and report-page breakpoint remains 720px.Screenshots
Screen.Recording.2026-09-02.at.4.37.40.PM.mov
Component-level screenshots from the two Storybook stories added in this PR (
Packages/Premium Analytics/UI/SectionHeader).Side by side, at 960px of header width:
Stacked, at 700px:
Notes for the reviewer
SectionHeader'stitlestays required. Left optional, a page could spread empty slots into the header and silently lose itsh1, which is what the video page did in its loading, error, and not-found states.h1around the skeleton, with a visually hidden “Loading…” label, and the header carriesaria-busywhile the summary resolves.DateRangerather than their own inline spellings of it..scrollArea,.content, and.widgets).