Skip to content
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

fix animateContentSize NavHost Modifier #315

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

tomczyn
Copy link

@tomczyn tomczyn commented Apr 25, 2024

Changed NavHost layout implementation so that BoxWithConstraints layout that wraps the NavHostContent will be composed only when there's a BackStackEntry to display.

There is an issue when the NavHost is inside a ModalBottomSheet, and the NavHost is wrapped by animateContentSize() Modifier. It interferes with showing animation of the bottom sheet, where it hangs for an additional ~600ms at the bottom of the screen. It appears that not composing the layout (BoxWithConstraints) when there's no BackStackEntry solves the issue.

Issue link: #313

Changes:

  • Added an if statement wrapping BoxWithConstraints so that it'll compose only when currentSceneEntry or currentFloatingEntry is not null.

We could avoid the if if not for the floating BackStackEntry which is also wrapped by the same BoxWithConstraints layout. If you have any idea how we could avoid it in some other way, feel free to update the PR with new changes. Or instead of nesting the rest of the logic inside the if we could invert it and do a return so the rest of the function won't run.

progress = 1f
state.snapTo(DragAnchors.Start)
}
if (currentSceneEntry != null || currentFloatingEntry != null) {
Copy link
Author

@tomczyn tomczyn Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be inverted if you prefer to avoid nesting, e.g.

if (currentSceneEntry == null && currentFloatingEntry == null) return

Copy link
Owner

@Tlaster Tlaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! LGTM!

@Tlaster Tlaster merged commit ced3a0e into Tlaster:master Apr 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants