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.

Sign up for GitHub

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

Jump to bottom

[Navigation] Introduce navigation-compose-material and support for bottom sheet navigation destinations #173

Closed
wants to merge 50 commits into from
Closed

[Navigation] Introduce navigation-compose-material and support for bottom sheet navigation destinations #173

wants to merge 50 commits into from

Conversation

jossiwolf
Copy link
Contributor

@jossiwolf jossiwolf commented May 3, 2021 •
edited
Loading

Proposed Changes

For easier integration with compose-material's ModalBottomSheetLayout, this library provides convenience wrappers and a BottomSheetNavigator to be able to declare and navigate to bottom sheet destinations, similar to bottom sheet dialog destinations in vanilla navigation.
BottomSheetNavigator drives a ModalBottomSheetLayout's sheetContent and state, controlling visibility according to the back stack and composing and disposing destinations that are navigated to or away from. BottomSheetNavigator offers the sheetContent as a composable that should be set as the ModalBottomSheetLayout's sheetContent.

  • Composable bottom sheet destinations can be added using NavGraphBuilder#bottomSheet
  • The BottomSheetNavigator needs to be registered with the NavController's navigatorProvider
  • Provides a convenience wrapper around ModalBottomSheetLayout that accepts a BottomSheetNavigator
  • Extracted BackStackEntryIdViewModel and the SaveableStateHolder.SaveableStateProvider extension into their own file in navigation-compose because they are needed in navigation-compose-material

Generating the API files for navigation-compose-material makes lint trip up about overloads/Java compatibility on a composable function (src/main/java/androidx/navigation/compose/material/BottomSheet.kt:35: error: A Kotlin method with default parameter values should be annotated with @JvmOverloads for better Java interoperability), so any pointers there would be appreciated :) as a workaround I added navigation-compose-material to the list of exceptions in AndroidXExtension#targetsJavaConsumers locally to generate the API files. That might even be the correct solution, let me know what you think is best.

I also thought about making navigation-compose-material a submodule of navigation-compose - would that be better?

Please let me know if you have ideas for more tests, I plan on adding a couple more, not super happy with them yet.

Cheers!

Testing

Test: Added tests for new behavior (BottomSheetNavigatorTest, SheetContentHostTest, NavGraphBuilderTest), ran with ./gradlew test connectedCheck. More tests to be added.
Demos: Introduced BottomSheetNavDemo

Issues Fixed

Fixes: b/180247978

@jossiwolf jossiwolf force-pushed the jw/compose-bottom-sheet-nav branch from 49579a4 to 09c9d7e Compare May 3, 2021 15:09
@dlam dlam requested a review from ianhanniballake May 3, 2021 15:36
@jossiwolf jossiwolf force-pushed the jw/compose-bottom-sheet-nav branch from 09c9d7e to 5a06c36 Compare May 3, 2021 19:27
@jossiwolf jossiwolf changed the title Introduce navigation-compose-material and support for bottom sheet navigation destinations [Navigation] Introduce navigation-compose-material and support for bottom sheet navigation destinations May 3, 2021
ianhanniballake
ianhanniballake requested changes May 4, 2021
View reviewed changes
Copy link
Member

@ianhanniballake ianhanniballake left a comment

Choose a reason for hiding this comment

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

Just a high level review at the moment - I hope to have some time to dive into the details in a later review (one CL huh? lol).

It looks like there's a few more infrastructural things (like the SaveableStateProvider) that we should split off into separate PRs that can be merged in prior to this. Let me know if you'd like to do this or if you'd like us to do this ahead of time.

I have some ideas on how we'd like to approach each of these if you'd like to reach out to me or just list out what dependencies you need here and I can comment on each.

@jossiwolf jossiwolf force-pushed the jw/compose-bottom-sheet-nav branch from c9fd9dc to 33a13f4 Compare May 4, 2021 15:55
@ianhanniballake ianhanniballake mentioned this pull request May 6, 2021
Closed
@jossiwolf jossiwolf force-pushed the jw/compose-bottom-sheet-nav branch 4 times, most recently from 4267618 to d0e4a20 Compare May 11, 2021 20:51
@jossiwolf jossiwolf force-pushed the jw/compose-bottom-sheet-nav branch 2 times, most recently from d67415c to 08f6fc5 Compare July 16, 2021 17:35
@googlebot
Copy link
Collaborator

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Jul 16, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@jossiwolf jossiwolf force-pushed the jw/compose-bottom-sheet-nav branch from a0022cb to 1e07c7a Compare July 16, 2021 17:42
@googlebot
Copy link
Collaborator

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@google-cla
Copy link

google-cla bot commented Jul 16, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@jossiwolf jossiwolf force-pushed the jw/compose-bottom-sheet-nav branch 3 times, most recently from 48a8a30 to a33a4b3 Compare July 24, 2021 15:20
@jossiwolf jossiwolf force-pushed the jw/compose-bottom-sheet-nav branch from 92e3c0f to d0a03a0 Compare July 28, 2021 18:29
jossiwolf added 15 commits August 1, 2021 17:06
@jossiwolf
@Ignore testSheetShownAfterNavControllerRestoresState
b533643 
Change-Id: I0f7e9f1d4d72c824977052d465cf45004a26cec9
@jossiwolf
Use rememberNavController over manually creating NavHostController be…
55fef09 
…cause of crash

Change-Id: I96cc1cf2eed55c5194905c41d2278de40fcaed4b
@jossiwolf
Use popWithTransition
df14817 
Change-Id: I149c313a44df7917e648548f01b2a09bf498281d
@jossiwolf
Make sheetState an impl detail of BottomSheetNavigator.kt and expose …
9bba09d 
…select properties through BottomSheetNavigatorState

Change-Id: Icdf496e0b68256034b25781f9d3254142807a686
@jossiwolf
Use markTransitionComplete API
9e98f25 
Change-Id: I5ab782fa46d0b02b55c82e69e5098c0ac25428be
@jossiwolf
Pin Compose 1.0.1 for navigation-compose-material
80f1029 
Change-Id: I945813b06cb945533fc0699bb5b54732da16e5e0
@jossiwolf
Use internalShow instead of show, the show call is cancelled
c09724b 
Change-Id: Ic7f6020509378d729d9e906235c2cba5d6c7da0f
@jossiwolf
Pop only dismissed sheet's entry
4e8677b 
Change-Id: I6017c5bfbbd438f92fc9ad36a3c09de1f5768ad5
@jossiwolf
Mark all entries' transitions as complete
cb6739f 
Change-Id: I1eb083a3c5b81485bdb1c33d2c8c6a07dbce82b0
@jossiwolf
Attach NavigatorState before first composition
88a78c5 
In this test, we were composing (and thus trying to access the BottomSheetNavigator's state) before we had attached the state to the navigator

Change-Id: I63309dd8b299ffafaa571226b091eb62ba1f3584
@jossiwolf
Lint :)
c66eb71 
Change-Id: I9bb5d36316c892930d742a72fcfd0e8b9b191dd9
@jossiwolf
Add testNavigateCompletesEntriesTransitions Test
82f8ba8 
Change-Id: If594ba4d1d5c884bbf8ef9f723e4a00c28e0ae62
@jossiwolf
Make BottomSheetNavigator's fields public
0e0046c 
Change-Id: I838fd686b37624f6f2e9da9b7da15fdec9b3f6ca
@jossiwolf
Add BottomSheetNavigator to NavController in composition
adcf347 
Change-Id: I1f03e990370dc421cca089f65d24399446979eb3
@jossiwolf
Remove testNavigationNestedInGraph test
f3423e1 
Change-Id: I4ae5d166118c55ec34d9a635660521d10b9afdc8
@jossiwolf jossiwolf force-pushed the jw/compose-bottom-sheet-nav branch from 793c180 to f3423e1 Compare August 1, 2021 15:52
@jossiwolf
Copy link
Contributor Author

Ayyyy whohoo! Thank you so much for sticking with this PR :) Gonna move over to Accompanist!

(btw, checks are failing because of Metalava. Hope it'll just magically work over there :D)

@jossiwolf jossiwolf closed this Aug 1, 2021
jossiwolf added a commit to jossiwolf/accompanist that referenced this pull request Aug 1, 2021
@jossiwolf
Import code from androidx/androidx#173
2ece706
jossiwolf added a commit to jossiwolf/accompanist that referenced this pull request Aug 1, 2021
@jossiwolf
Import code from androidx/androidx#173
7a8c370
@jossiwolf jossiwolf mentioned this pull request Aug 1, 2021
Closed
jossiwolf added a commit to jossiwolf/accompanist that referenced this pull request Aug 1, 2021
@jossiwolf
Import code from androidx/androidx#173
9285cb4
jossiwolf added a commit to jossiwolf/accompanist that referenced this pull request Aug 1, 2021
@jossiwolf
Import code from androidx/androidx#173
3675914
jossiwolf added a commit to jossiwolf/accompanist that referenced this pull request Aug 1, 2021
@jossiwolf
Import code from androidx/androidx#173
0f2fc7a
@jossiwolf jossiwolf mentioned this pull request Aug 1, 2021
Merged
jossiwolf added a commit to jossiwolf/accompanist that referenced this pull request Aug 1, 2021
@jossiwolf
Import code from androidx/androidx#173
376fa74
jossiwolf added a commit to jossiwolf/accompanist that referenced this pull request Aug 2, 2021
@jossiwolf
Import code from androidx/androidx#173
9591d8f
jossiwolf added a commit to jossiwolf/accompanist that referenced this pull request Aug 3, 2021
@jossiwolf
Import code from androidx/androidx#173
0c7d080
chrisbanes pushed a commit to google/accompanist that referenced this pull request Aug 3, 2021
@jossiwolf
[Navigation] Add Navigation-Material (#592)
9b4b937 
* Introduce Navigation-Compose-Material Module

* Import code from androidx/androidx#173

* Use try-finally to set `hideCalled` flag

* Run spotless

* Introduce ExperimentalMaterialNavigationApi.kt

* Update docs and sample

* Remove unused import

* Update API after introducing ExperimentalNavigationMaterialApi

* Update tests to opt-in to experimental APIs

* Add dummy tests to help with sharding

* Format dummy tests :)
@jossiwolf jossiwolf mentioned this pull request Aug 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianhanniballake ianhanniballake

@chrisfillmore chrisfillmore

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jossiwolf @googlebot @ianhanniballake @chrisfillmore