-
Notifications
You must be signed in to change notification settings - Fork 537
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
PageHeader: Fixes layout issues on title and sub components #4669
Conversation
🦋 Changeset detectedLatest commit: 4ce72c2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
@@ -435,6 +395,9 @@ const Breadcrumbs: React.FC<React.PropsWithChildren<ChildrenPropTypes>> = ({ | |||
return value ? 'none' : 'flex' | |||
}), | |||
alignItems: 'center', | |||
fontWeight: 'initial', | |||
lineHeight: 'var(--text-body-lineHeight-large, 1.5)', |
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.
Shouldn't we be using var(--text-body-lineHeight-medium, 1.4285)
?
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.
Oh sorry I missed to update that! Just pushed it.
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.
@mperrotti yeah it is interesting! I can confirm that I see the same thing as you shared. It might be because of the font settings etc are different where these snapshots are generated. |
Important
For release coordinator: I would need two commits (commit1, commit2) to be added to the upgrade PR
Reported internally on Slack (Hubbers link only) that there is a layout shift happening on titles and sub components. (font-size (for titles) and the height of the actions change after the initial render)
Video describes the layout shift issue in the prod storybook and how this PR fixes it on the local environment. Especially the actions on the right side jumps quite visibly.
Rewatch.Screen.Recording.-.2024-06-10.at.1.57.mp4
Changelog
New
Changed
data-component="TitleArea"
and each related variant data attribute, and sets the font styling and height for sub components (leading actions, actions etc) for each variant. Since we don't need to wait for any render as we did before, we don't see any layout shift.TitleArea
(as described just above), we don't hit specificity issues when there is an override on the Title (like the reported issue), therefore no layout shift on the titles either.Removed
variant
value fromPageHeader.TitleArea
in the useeffect.Rollout strategy
Testing & Reviewing
Merge checklist