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

Enable to use a BottomNavigator scoped to a Fragment and tied to its child FragmentManager #26

Closed
wants to merge 2 commits into from

Conversation

qnga
Copy link

@qnga qnga commented Apr 21, 2021

Hello!

I've added the ability of properly using a BottomNavigator inside a Fragment instead of an Activity. It implies to execute transactions with the Fragment's child FragmentManager, tie the execution of commands to its lifecycle and scope the BottomNavigator instance to its ViewModelStore.

For the sake of simplicity and backward compatibility, I've overloaded a few methods with alternatives that take a Fragment in argument.

This PR would fix #13.

Copy link
Contributor

@guelo guelo left a comment

Choose a reason for hiding this comment

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

Thank you so much for your submission! I think the code looks good but in order to merge I need 3 things.

  • Unit tests for the new onCreate functions. You should be able to copy similar tests in BottomNavigationTest.
  • A demo in the sample app that exercises the child fragments. Maybe one of the existing tabs could have a nested BottomNavigator? The sample app is used both as a demo and a platform for espresso tests. A simple espresso test is fine, it doesn't have to be comprehensive.
  • Some documentation about this in README.md

Is this work that you can do?

@qnga
Copy link
Author

qnga commented May 5, 2021

  • I'm not sure which unit tests I should replicate with Fragments. BottomNavigator.onCreate is just a public wrapper I can't find specific tests for.
  • A nested BottomNavigator would look quite weird, wouldn'it? Rather, I would have thought of a different Activity. For testing purpose it might be fine though.
  • I made a try. Is it enough?

@qnga qnga closed this by deleting the head repository Mar 9, 2024
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.

Can the BottomNavigator operate when placed in a Fragment?
2 participants