-
Notifications
You must be signed in to change notification settings - Fork 667
UnderlineNav: Fix flickering by moving icon visibility control to pure CSS
#8108
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
base: main
Are you sure you want to change the base?
Changes from all commits
a789e19
302d679
6fac497
a10a95c
a1f49a5
5076e18
8965e7c
f755913
15d64eb
b79ae2e
5614068
6b360c1
2c8e848
c9c8c00
b3e85f5
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 |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@primer/react": patch | ||
| --- | ||
|
|
||
| `UnderlineNav`: Fix icon flickering on some screen sizes before initial render/hydration |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,15 +2,13 @@ | |
| /* Progressive enhancement: Detect overflow using scroll-based animations. | ||
| The idiomatic way would be a scroll-state container query but browser support | ||
| is slightly better for animations. */ | ||
| animation: detect-overflow linear; | ||
| animation-timeline: scroll(self block); | ||
| animation: | ||
| detect-overflow linear, | ||
| delay-visibility forwards 0ms 5ms; | ||
| animation-timeline: scroll(self block), auto; | ||
|
|
||
| --UnderlineNav_moreButton-visibility: hidden; | ||
| --UnderlineNav_icons-display: inline; | ||
|
|
||
| &[data-hide-icons='true'] { | ||
| --UnderlineNav_icons-display: none; | ||
| } | ||
| --UnderlineNav_hide-icons-play-state: paused; | ||
|
|
||
| &[data-has-overflow='true'] { | ||
| --UnderlineNav_moreButton-visibility: visible; | ||
|
|
@@ -21,12 +19,45 @@ | |
| 0%, | ||
| 100% { | ||
| --UnderlineNav_moreButton-visibility: visible; | ||
| --UnderlineNav_icons-display: none; | ||
| --UnderlineNav_hide-icons-play-state: running; | ||
| } | ||
| } | ||
|
|
||
| .ItemsList [data-component='icon'] { | ||
| display: var(--UnderlineNav_icons-display); | ||
| /* Unlike the more button, the icons are removed from the layout when hidden. This can cause the container to no | ||
| longer overflow, which would cause a flickering loop if we just drove the visibility directly from the scroll state. | ||
| So instead we drive an animation that can only play forwards so the icons stay hidden for the life of the page, even | ||
| if the container is no longer overflowing. We can't just make the scroll-driven animation itself play forwards, | ||
| because scroll-driven animations are conditionally applied and revert when not scrollable. */ | ||
| animation-name: hide-icons; | ||
| animation-fill-mode: forwards; | ||
| animation-timing-function: linear; | ||
| animation-play-state: var(--UnderlineNav_hide-icons-play-state); | ||
| animation-duration: 0.1ms; /* must be greater than 0 */ | ||
| } | ||
|
|
||
| /* Slow devices will still flicker on initial paint as we figure out if we have room to render icons or not, | ||
| so we delay the tabs visibility by a very short time to allow things to settle. */ | ||
| @keyframes delay-visibility { | ||
| 0%, | ||
| 99.9% { | ||
| visibility: hidden; | ||
| } | ||
|
|
||
| 100% { | ||
| visibility: visible; | ||
| } | ||
| } | ||
|
|
||
| @keyframes hide-icons { | ||
| 0% { | ||
| display: inline; | ||
| } | ||
|
|
||
| 0.1%, | ||
| 100% { | ||
| display: none; | ||
| } | ||
| } | ||
|
Comment on lines
+52
to
61
Contributor
Author
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. I'm like 99% sure this is incorrect. Browsers will animate the property but (obviously) not smoothly; they'll discretely step from one value to the next.
Member
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. Checking https://primer-770a2beee9-13348165.drafts.github.io/storybook/?path=/story/components-underlinenav-features--with-icons I don't think the icons are hiding
Contributor
Author
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. This turned out to be due to not setting an initial value for the variable. Which feels like a browser bug, but it's easy to solve by initializing it. |
||
|
|
||
| .MoreButtonContainer { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,7 @@ | |
|
|
||
| .UnderlineItemList { | ||
| flex-wrap: wrap; | ||
| overflow: hidden; | ||
| flex: 1; | ||
|
Contributor
Author
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.
|
||
| } | ||
| } | ||
| } | ||
|
|
||
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.
Interestingly, this can't be arbitrarily small;
0.0001msis treated as0in Chrome.0.1msseems to work very consistently though.I did try making the duration dynamic, instead of the play state, and setting the duration to
0to immediately resolve the animation when I want to toggle it. That didn't work though; the style didn't 'stick' if the duration was increased again.