-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
feat(pageframe): adopt TopBar.Slot in Layout.Title
#112515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import {Tabs} from '@sentry/scraps/tabs'; | |
|
|
||
| import {usePrimaryNavigation} from 'sentry/views/navigation/primaryNavigationContext'; | ||
| import {SecondaryNavigationContext} from 'sentry/views/navigation/secondaryNavigationContext'; | ||
| import {TopBar} from 'sentry/views/navigation/topBar'; | ||
| import {useHasPageFrameFeature} from 'sentry/views/navigation/useHasPageFrameFeature'; | ||
|
|
||
| /** | ||
|
|
@@ -137,7 +138,16 @@ export const HeaderActions = styled('div')` | |
| * Includes flex gap for additional items placed with the text (such as feature | ||
| * badges or ID badges) | ||
| */ | ||
| export const Title = styled('h1')<{withMargins?: boolean}>` | ||
| export function Title(props: React.ComponentProps<typeof LegacyTitle>) { | ||
| const hasPageFrame = useHasPageFrameFeature(); | ||
|
|
||
| if (hasPageFrame) { | ||
| return <TopBar.Slot name="title">{props.children}</TopBar.Slot>; | ||
| } | ||
|
Comment on lines
+144
to
+146
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The Suggested FixUpdate the Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews. |
||
|
|
||
| return <LegacyTitle {...props} />; | ||
| } | ||
| const LegacyTitle = styled('h1')<{withMargins?: boolean}>` | ||
| width: 100%; | ||
| white-space: nowrap; | ||
| overflow: hidden; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: A second
TopBar.Slotwith the name 'title' is registered on Settings pages, causing a conflict that hides either the breadcrumbs or the page title.Severity: MEDIUM
Suggested Fix
Conditionally render the
TopBar.SlotwithinLayout.Titleto avoid registering it on pages where the 'title' slot is already in use, such as the Settings pages. Alternatively, use a different, non-conflicting slot name.Prompt for AI Agent
Did we get this right? 👍 / 👎 to inform future reviews.