search

google / accompanist

A collection of extension libraries for Jetpack Compose
https://google.github.io/accompanist
Apache License 2.0
7.38k stars 596 forks source link

[navigation-material] expose sheetGesturesEnabled flag #1642

Closed amanv8060 closed 6 months ago

amanv8060 commented 1 year ago

Fixes #730

Expose sheetGesturesEnabled Flag

ianhanniballake commented 1 year ago

Similar to the comment on #1342, we'd want to make this available on a per destination level, not a global flag.

This may be easier said than done given the current state of ModalBottomSheetLayout (also as explained in that comment), but if it is possible, we'd be happy to merge a destination level flag in if you'd like to put up a new patch set.

amanv8060 commented 1 year ago

Will give it a try, although it doesn't look easily possible.

amanv8060 commented 1 year ago

@ianhanniballake, I have added a destination-level flag and updated the bottom sheet sample to demonstrate the same. I am a little unsure about the performance implications of this approach.

Can you take a look and let me know what you think?

If this looks good, I can try adding tests for this.

jossiwolf commented 1 year ago

@amanv8060, thanks for the PR!

I'd be interested to see if tests for the following scenarios (see the comment Ian linked) would pass:

The concerns are about recreating the state in time when the value changes as the destination changes, but maybe it'd work. We'd want some good test coverage that tests

Navigating from a skipHalfExpanded = false to a skipHalfExpanded = false destination Navigating from a skipHalfExpanded = false to a skipHalfExpanded = true destination Navigating from a skipHalfExpanded = true to a skipHalfExpanded = false destination and ensures that the sheet opens up to the correct state.

If yes, we can look at performance a bit more. What concerns do you have here?

amanv8060 commented 1 year ago

I will try adding tests for these cases.

amanv8060 commented 1 year ago

@jossiwolf I have added basic tests, I can't test the sheet state like in the case of skipHalfExpanded( since this value isn't stored in sheetstate) so I have added tests to ensure that the stateflow is indeed emitting correct values in all the mentioned cases.

amanv8060 commented 1 year ago

cc/ @jossiwolf In case you missed it.

akashnimare commented 1 year ago

Would love to see this merged ✌️

VahidGarousi commented 11 months ago

Would love to see this merged ✌️

ianhanniballake commented 6 months ago

With the release of Compose Material 1.7.0-alpha04, the Material team has added a new artifact: androidx.compose.material:material-navigation, which fully replaces Accompanist Navigation Material.

As such, we are closing all PRs here on Accompanist.