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

Improvement/navigation architecture #73

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

gabrielpozo
Copy link
Collaborator

Nombre del branch:

navigation-architecture

Descripcion

-pass the button click action to the ViewModel in order to make it testable
-remove navigation bar
-add fragnav library https://github.com/ncapdevi/FragNav

Pull request checklist:

  • El codigo incluye Unit Test
  • El codigo compila almenos 2 veces antes de crear este pull request
  • He realizado cambios en la wiki si es necesario
  • El codigo incluido puede romper la app
  • has avisado al equipo en Slack sobre el Pull Request

…into improvement/navigation-architecture

# Conflicts:
#	app/src/main/java/com/architectcoders/equipocinco/ui/fragment/master/child/FavouriteMoviesFragment.kt
#	app/src/main/res/layout/activity_main.xml
#	presentation/src/main/java/com/architectcoders/presentation/viewmodels/MovieViewModel.kt
bundleOf(DetailMovieFragment.MOVIE_ID_KEY to it.id)
)
}
adapter = MovieAdapter(items.toMutableList(), viewModel::onSelectedMovie)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

aqui es donde me referia que no rastreabamos la accion del onClick en una peli. Ahora si que le pasamos al viewModel la accion(onSelectedMovie) y de esta manera ya podemos testear este logica-flow

Copy link
Collaborator

Choose a reason for hiding this comment

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

Mola!

@@ -65,6 +73,12 @@ class MovieViewModel(
}
}

fun onSelectedMovie(movie: Movie) {
_modelNavigation.value = Event(
NavigationModel(movie)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

he añadido Event aqui, de otro modo cuando vuelva atras a Movies desde Details Screen, ya se ha consumido y no vuelva de nuevo a Details instantaneamente

R.anim.slide_in_from_left,
R.anim.slide_out_to_right
).build()
fragmentHideStrategy = FragNavController.DETACH_ON_NAVIGATE_HIDE_ON_SWITCH
Copy link
Collaborator Author

@gabrielpozo gabrielpozo Feb 26, 2020

Choose a reason for hiding this comment

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

Aqui puedes personalizar la estrategia del stack de fragments, ahora tenemos DETACH_ON_NAVIGATE_HIDE_ON_SWITCH que basicamente significa q en nuestro caso siempre tendria 3 vistas en memorias a la vez, no importa en que nivel de profundidad estes. Por ejemplo: si estas en detailMovie del TAB de popularMovies, tendrías en memoria estas 3, DetailScreen(de popularMovie), TopRatedScreen y FavouriteScreen , y si le das hacia atras desde DetailScreen, tendrias ahora estas vistas PopularMovie, TopRatedScreen y FavouriteScreen en memoria

Copy link
Collaborator Author

@gabrielpozo gabrielpozo Feb 26, 2020

Choose a reason for hiding this comment

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

cuando vas en niveles dentro de un mismo TAB, por ej. desde PopularMovies a detail, se hace un detach del fragment de modo que se destruye la View del fragment de PopularMocies pero queda la instancia del objeto en memoria, y en consecuencia el ViewModel sobrevive tambien conteniendo en sus LiveDatas los ultimos valores. Por eso tuve que hacer un wrapper del object Navigation en un Event en el onSelectedMovie y preguntar si ya se habia consumido anteriormente, de otro modo me volveria de nuevo al detailscreen y no me dejaria ir nunca hacia atras

Copy link
Collaborator

Choose a reason for hiding this comment

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

Brutal! Gracias por la explicación!

class FavouriteMoviesFragment : MoviesFragment(){
class FavouriteMoviesFragment : MoviesFragment() {

companion object;
Copy link
Collaborator

Choose a reason for hiding this comment

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

¿Esto para qué se usa? Por cierto, no sabía que se podía declarar así, en vez de llaves, concerrando con punto y coma. He visto que si no se pusiera, da un error de compilación.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perdona Gabi por la tardanza, es simplemente para indicar que tenemos una extension del companion object en otro archivo. Si te das cuenta tenemos un newInstance para crear el objeto moviesFragment en uno de los extension files. Kotlin te obliga a q pingas companion object para dejar saber q hay una parte estatica inplementada en otro lado

Copy link
Collaborator Author

@gabrielpozo gabrielpozo Feb 29, 2020

Choose a reason for hiding this comment

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

En resumen lo puse para no poner el newInstance en el fragment y dejarlo mas limpio y legible

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perfect! No tenía ni idea. Muchas gracias por la explicación! 😊

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yo tampoco tenía ni idea! =O Gracias por la explicación caballero!

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.

3 participants