Navigator: hide scrollbars while animating
#35317
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ciampo
added
[Type] Bug
ciampo
commented
Oct 4, 2021
|
|
||
| return { | ||
| push( path, options ) { | ||
| setPath( { path, ...options } ); | ||
| setIsAnimating( true ); | ||
| setNavigatorPath( { path, ...options } ); |
There was a problem hiding this comment.
This approach seems to work better than using the onAnimationStart callback on the motion.div component:
- in my tests, the
onAnimationStartcallback would fire after the animation starts, which caused the scrollbars to still appear from time to time (depending on the browser rendering loop) - setting
isAnimatingtotruein thepushfunction ensures that the flag is applied before the animation starts
6 tasks
|
Size Change: +113 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
This was referenced Oct 4, 2021
stokesman
reviewed
Oct 4, 2021
packages/components/src/navigator/navigator-provider/component.tsx
Outdated
Show resolved
Hide resolved
This was referenced Oct 4, 2021
Base automatically changed from
feat/components-navigator-typescript-refactor
to
trunk
October 5, 2021 15:13
ciampo
force-pushed
the
fix/components-navigator-remove-scrollbars-while-transitioning
branch
from
d0e3e85
to
0c21e52
Compare
ciampo
force-pushed
the
fix/components-navigator-remove-scrollbars-while-transitioning
branch
from
0c21e52
to
5a17e72
Compare
|
Addressed the initial round of feedback and rebase on top of |
|
Hey @youknowriad @diegohaz @mirka @ntsekouras and @jorgefilipecosta, I'd love to hear your opinion about the approach taken in this PR and how it compares against #35332.
What do you think? We could, for example, opt for the simpler solution #35332 and come up with a more elaborate solution only if necessary |
7 tasks
|
The advantage I see here is less about popovers (they should be portal'ed anyway like @stokesman says in #35332 (comment)), but in scenarios where the consumer inadvertently causes x-overflows with naive CSS and dynamic content (translations, tables, etc). This isn't an uncommon occurrence, so in those cases I would want the overflow to be at least scrollable. I find the slightly added complexity to be an acceptable trade off for that tolerance. |
That is a fair point. @stokesman has updated #35332 to potentially address this issue as well. With regards on how to proceed, I'm happy with both solutions (given that they solve the issue equally well). I'm happy to follow whatever decision is made here. |
|
Let's go with #35332 for now |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR builds on top of #35214 and proposes a fix for the issue raised in #34904 (review) about the scrollbars appearing during screen animations.
The proposed solution applies the
overflow: hiddenstyles on theNavigatorProviderwrapper only while screens are animating (which addresses the concerns expressed in #35214 (comment)).The main changes are:
NavigatorContextfrom array to object, in order to include theisAnimating/setInsAnimatingprops.isAnimating/setInAnimatingprops, apply theoverflow: hiddenstyles to the wrapperdivonly while an animation is in progress.Flyoutto show that the overflow rule doesn't affect modalsHow has this been tested?
animationEnterDurationandanimationExitDurationvariables could be changed to a higher value (e.g.5).Screenshots
Before
navigator-scrollbars.mp4
After
Kapture.2021-10-04.at.12.32.46.mp4
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).