Skip to content

Use PageHeader title on list pages#2283

Closed
elizabetdev wants to merge 1 commit into
agent/page-shellfrom
agent/page-migrate-lists
Closed

Use PageHeader title on list pages#2283
elizabetdev wants to merge 1 commit into
agent/page-shellfrom
agent/page-migrate-lists

Conversation

@elizabetdev
Copy link
Copy Markdown
Contributor

Summary

  • Switches Alerts, Dashboards, and Saved Searches to the explicit PageHeader title prop for consistency with the shared header API.

Test plan

  • Open Alerts, Dashboards list, and Saved Searches; confirm titles and layout unchanged aside from header implementation

Made with Cursor

Aligns Alerts, Dashboards, and Saved Searches with the shared header API.

Co-authored-by: Cursor <cursoragent@cursor.com>
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 15, 2026

⚠️ No Changeset found

Latest commit: 151acc0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link
Copy Markdown

vercel Bot commented May 15, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment May 15, 2026 0:00am

Request Review

@github-actions
Copy link
Copy Markdown
Contributor

<!-- claude-code-review -->

PR Review

✅ No critical issues found.

Simple, well-scoped migration of three list pages (AlertsPage, DashboardsListPage, SavedSearchesListPage) from the children-based <PageHeader>Title</PageHeader> form to the explicit title prop. The PageHeader component supports both shapes; the title-prop branch wraps the text in a semantic <h1> with CSS that inherits font/weight/line-height, so visual output should be unchanged while gaining proper heading semantics.

Minor (non-blocking):

  • The PR description's test plan checkbox is unchecked — worth visually confirming the three pages render identically before merge, since this changes the rendered DOM (adds wrapping .start div and <h1>).

@github-actions
Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

This diff is a 6-line mechanical migration of three list pages from the children form of PageHeader to the explicit title prop. PageHeader already accepts both shapes (title?: string and children?: React.ReactNode in packages/app/src/components/PageHeader.tsx:5-21), so the call sites remain valid and the page output now flows through the documented <h1 className={styles.title}> toolbar path. TeamPage.tsx legitimately continues to use the children shape because it renders an editable team name — that is the use case the children slot is documented for, so leaving it unmigrated is the right call rather than an oversight.

One thing worth verifying manually since the test plan is visual-only: in the previous rendering the heading text was a raw text child inside <header> with no <h1> wrapper, whereas after this change each page renders an actual <h1> inside the toolbar layout. That is a semantic improvement (real heading landmark) but it may shift vertical spacing / typography slightly depending on PageHeader.module.scss. The PR's own test plan already covers this (open each page and confirm layout unchanged), so no review action needed — just don't skip that step.


Reviewers (4): correctness, testing, maintainability, project-standards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant