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

[Jetsnack] Fixes #1137 - LazyColumn last element partially visible #1149

Conversation

rool78
Copy link
Contributor

@rool78 rool78 commented Jun 23, 2023

@rool78 rool78 requested a review from a team as a code owner June 23, 2023 20:28
@google-cla
Copy link

google-cla bot commented Jun 23, 2023

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.

Copy link

@style96 style96 left a 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.

@rool78
Copy link
Contributor Author

rool78 commented Jun 24, 2023

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 🙂

@riggaroo
Copy link
Collaborator

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 shouldShowBottomBar variable all together, and placing the Scaffold (and therefore Bottom navigation) in each Composable that requires it instead.

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.
Passing around padding values is also not great for long term maintenance of the screens, you'd need to keep track of where or where not to use the values.

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!

@rool78
Copy link
Contributor Author

rool78 commented Jun 26, 2023

Thanks @riggaroo. Makes sense! I can update this PR

@rool78
Copy link
Contributor Author

rool78 commented Jun 26, 2023

PR was updated with @riggaroo suggested approach, a review would be really appreciated 🙂

navigateToRoute = appState::navigateToBottomBarRoute
)
}
},
snackbarHost = {
SnackbarHost(
Copy link
Collaborator

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.

Copy link
Contributor Author

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())
                }
            }
        }
    }
}

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@riggaroo
Copy link
Collaborator

@rool78 thanks so much - you need to run spotless to make the build pass :)

@rool78
Copy link
Contributor Author

rool78 commented Jul 11, 2023

@riggaroo done! could we relaunch workflows? :)

Copy link
Collaborator

@riggaroo riggaroo 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 contritbution.

@riggaroo riggaroo merged commit 0205e1b into android:main Jul 13, 2023
3 checks passed
@akaSourav
Copy link

Seems like placing Bottombar in each Scaffold ruins the bottomnavigationbar animation. The Animation isn't smooth anymore.

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.

[Jetsnack]: Navigate case LazyColumn last element partially visible.
4 participants