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

Update: Navigator injected on TvShowCatalogPresenter #7

Conversation

JorgeCastilloPrz
Copy link
Contributor

Navigator is now injected into the TvShowCatalogFragmentPresenter. The presenter's method "onTvShowThumbnailClicked" is now using navigator to move the user into the corresponding tvshow details.

All the navigation logic has been moved into the Navigator, even the fragment related one. The main problem here is that you need to make Navigator know everything about potentially available Fragments in the current injected Activity context.

Aproach PR made for discussion!

@pedrovgs
Copy link
Owner

As you says, now your Navigator should know about your fragments implementation details. But this is not a problem!

Think about navigation, inside your system the navigation should be implemented like a boundary. To improve this code Navigator should be an interface and be implemented by TvShowsNavigator or AndroidNavigator or whatever you want for your app. Implementation details of the same boundary, the android application boundary in your case, can be coupled. If your navigation changes, you'll have to change your Navigator implementation.

To me this code is an improvement :) Thanks @JorgeCastilloPrz. Next time, take care about the code style :P

pedrovgs added a commit that referenced this pull request Feb 12, 2015
…_view_impl_for_navigation

Update: Navigator injected on TvShowCatalogPresenter
@pedrovgs pedrovgs merged commit 90b6eb2 into pedrovgs:master Feb 12, 2015
@wanyt wanyt mentioned this pull request Aug 9, 2016
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.

None yet

2 participants