Use base-size-32 instead of hardcoded 32px for item height#8188
Use base-size-32 instead of hardcoded 32px for item height#8188iansan5653 wants to merge 3 commits into
base-size-32 instead of hardcoded 32px for item height#8188Conversation
🦋 Changeset detectedLatest commit: 29cfb51 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 |
github-actions
Bot
added
the
integration-tests: recommended
|
Fixes an issue with `UnderlineNav` items overflowing in Safari due to text size overrides.
There was a problem hiding this comment.
Pull request overview
Adjusts UnderlineNav overflow behavior to better handle Safari text size overrides by aligning tab item sizing with Primer tokens and making overflow detection more tolerant of minor clipping.
Changes:
- Replaced a hardcoded
32pxheight withvar(--base-size-32)for underline nav items. - Relaxed overflow detection to allow up to 5% clipping (
intersectionRatiothreshold from1to0.95). - Added a patch changeset describing the Safari overflow fix.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/internal/components/UnderlineTabbedInterface.module.css | Uses --base-size-32 instead of a hardcoded 32px for underline item height. |
| packages/react/src/internal/components/OverflowObserverProvider.tsx | Makes overflow detection less strict (0.95 threshold) to reduce false positives from layout rounding issues. |
| .changeset/heavy-brooms-wash.md | Patch changeset entry for the UnderlineNav Safari overflow behavior fix. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Low
| } | ||
| }, | ||
| {root, threshold: [0, 1]}, | ||
| {root, threshold: [0, 0.95]}, |
| * Treat any target that is not fully visible within the observer root as overflowing. Unwrapped items should be fully | ||
| * visible, but in some scenarios the layout isn't perfectly accurate (ie, in Safari with extra-large text size) so we | ||
| * allow for up to 5% to be clipped before hiding the item. |
| function getIsOverflowing(entry: Pick<IntersectionObserverEntry, 'intersectionRatio' | 'isIntersecting'>) { | ||
| return !entry.isIntersecting || entry.intersectionRatio < 1 | ||
| return !entry.isIntersecting || entry.intersectionRatio < 0.95 |
|
Confirmed working in storybook on every text size Screen.Recording.2026-07-17.at.2.00.17.PM.mov |
|
Integration test results from github/github-ui PR:
All checks passed! |
The underline nav item height is hardcoded at 32px. This means that when the font size changes (ie, "Make text smaller" in Safari), the item height doesn't respect the new font size. But the container does, because it has
max-height: var(--control-xlarge-size)which is font-size dynamic. This causes the container to clip all the items by having a smaller height than they can fit in, which treats them all as overflowing.The fix is simple, we can just use
--base-size-32instead.However, this still doesn't work perfectly. On sufficiently large font sizes (+3 zoom levels), the pixel-perfect accuracy of the sizes just isn't calculated quite right, and so the items still overflow even though they seem fully visible. I think this is a safari issue and there's not much we can do about it except to make the overflow logic slightly more forgiving, allowing up to 5% of an item to be clipped before hiding it.
Changelog
Changed
UnderlineNavitems all being considered overflowing on Safari with font size overridesRollout strategy
Testing & Reviewing