-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
[Jetsnack] Fixes #1137 - LazyColumn last element partially visible #1149
[Jetsnack] Fixes #1137 - LazyColumn last element partially visible #1149
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
I checked changes. It worked well with api v28 phone and v33 phone.
Thanks @style96! I also saw that you solved it in this PR. Maybe your solution is simpler, wdyt? Let's also see @florina-muntenescu thoughts 🙂 |
Thanks @rool78 and @style96 for the detailed pull requests and discussion. Appreciate your work here, after looking at this with the team, we believe the correct fix would be to remove the The dialog approach, although it fixes the problem, its a patch that'd be exposed again later if a new composable is introduced that does a similar thing to the current SnackDetail composable. Please let us know if either of you would like to update or submit a new PR for the change I've described :) Thanks again for the help! |
Thanks @riggaroo. Makes sense! I can update this PR |
PR was updated with @riggaroo suggested approach, a review would be really appreciated 🙂 |
navigateToRoute = appState::navigateToBottomBarRoute | ||
) | ||
} | ||
}, | ||
snackbarHost = { | ||
SnackbarHost( |
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.
The Snackbars are now obscuring the content of BottomBar, I think it would be good to include the SnackbarHost per Scaffold too, where required.
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.
Good catch! It's because scaffoldState is managed at UI tree root level (JetsnackApp) and we removed the bottom bar there. In my last commit I'm using rememberJetsnackAppState
in Cart composable to access scaffoldSate, I think is a bit overkill as I don't need access to the navController (anyway it's solving the problem). I also don't like passing the scaffoldState down the UI tree. Maybe I could refactor JetsnackAppState
removing scaffoldState from it and create a custom JetsnackScaffoldState
. Something like this, what do you think?
/**
* Remember and created an instance of [JetsnackScaffoldState]
*/
@Composable
fun rememberJetsnackScaffoldState(
scaffoldState: ScaffoldState = rememberScaffoldState(),
snackbarManager: SnackbarManager = SnackbarManager,
resources: Resources = resources(),
coroutineScope: CoroutineScope = rememberCoroutineScope()
): JetsnackScaffoldState = remember(scaffoldState, snackbarManager, resources, coroutineScope) {
JetsnackScaffoldState(scaffoldState, snackbarManager, resources, coroutineScope)
}
/**
* Responsible for holding [ScaffoldState], handles the logic of showing snackbar messages
*/
@Stable
class JetsnackScaffoldState(
val scaffoldState: ScaffoldState,
private val snackbarManager: SnackbarManager,
private val resources: Resources,
coroutineScope: CoroutineScope
) {
// Process snackbars coming from SnackbarManager
init {
coroutineScope.launch {
snackbarManager.messages.collect { currentMessages ->
if (currentMessages.isNotEmpty()) {
val message = currentMessages[0]
val text = resources.getText(message.messageId)
// Notify the SnackbarManager so it can remove the current message from the list
snackbarManager.setMessageShown(message.id)
// Display the snackbar on the screen. `showSnackbar` is a function
// that suspends until the snackbar disappears from the screen
scaffoldState.snackbarHostState.showSnackbar(text.toString())
}
}
}
}
}
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.
Yeah the proposed change will be better I agree.
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.
PR updated with the change.
@rool78 thanks so much - you need to run spotless to make the build pass :) |
@riggaroo done! could we relaunch workflows? :) |
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.
Thanks for the contritbution.
Seems like placing Bottombar in each Scaffold ruins the bottomnavigationbar animation. The Animation isn't smooth anymore. |
Fixes #1137
Current:
Jetsnack-1137-current.webm
Fix:
Jetsnack-1137-fix.webm