Migrate GalleryFragment and GardenFragment to Compose #819
Conversation
Change-Id: I0563a0ae66e807b0ddb6ed335adf619a0a21b9cf
Change-Id: I9c36a93116944e1b504e42e6070743922d3f1bd7
arriolac
force-pushed
the
chris/feat/816
branch
from
d9502f9
to
5a386e6
Compare
Change-Id: I5e92ee75ec6d6982b4f48940fbc4d289a28f791d
Change-Id: Ia9a25082eca36f099c6416d0781c01fe7aed7dec
arriolac
changed the title
Change-Id: I0a10b9ab5515d380232cc173c962acc8c62aece4
Change-Id: I6d894646e8e7f266b72a8f49848e2c9c69de6efa
| onAddPlantClick: () -> Unit, | ||
| onPlantClick: (PlantAndGardenPlantings) -> Unit | ||
| ) { | ||
| val gardenPlants = viewModel.plantAndGardenPlantings.observeAsState().value |
There was a problem hiding this comment.
What do you think of directly using Flow in the viewmodel instead of mapping to LiveData?
This way we can skip one mapping, so instead of Flow -> LiveData -> State, we'd have just Flow -> State
There was a problem hiding this comment.
Yep - agreed that's more straightforward
| ReportDrawnWhen { | ||
| gardenPlants != null | ||
| } |
There was a problem hiding this comment.
I think you could add a more comment that it covers both empty and not empty scenario.
Alternatively, you could also call it within the if to be more readable (that once it's reported when the list is empty and once when list is not empty)
app/src/main/java/com/google/samples/apps/sunflower/compose/garden/GardenScreen.kt
Show resolved
Hide resolved
| text = vm.plantName, | ||
| Modifier | ||
| .padding( | ||
| top = dimensionResource(id = R.dimen.margin_normal), |
There was a problem hiding this comment.
would it be worth to extract the dimension into a variable to prevent parsing it multiple times?
| @@ -315,7 +330,7 @@ private fun PlantImage( | |||
| override fun onLoadFailed( | |||
| e: GlideException?, | |||
| model: Any?, | |||
| target: Target<Drawable>?, | |||
| target: com.bumptech.glide.request.target.Target<Drawable>?, | |||
There was a problem hiding this comment.
can we import the Target instead of the full qualifier here? (same 👇🏼 )
app/src/main/res/values/styles.xml
Outdated
| @@ -20,7 +20,8 @@ | |||
| <style name="Base.Theme.Sunflower" parent="Theme.MaterialComponents.DayNight.NoActionBar"> | |||
| <item name="colorPrimary">@color/sunflower_green_500</item> | |||
| <item name="colorPrimaryVariant">@color/sunflower_green_700</item> | |||
| <item name="colorOnPrimary">@color/sunflower_yellow_500</item> | |||
| <!-- <item name="colorOnPrimary">@color/sunflower_yellow_500</item>--> | |||
| @@ -551,12 +584,14 @@ private fun PlantDescription(description: String) { | |||
| @Preview | |||
| @Composable | |||
| private fun PlantDetailContentPreview() { | |||
| // FIXME: Preview is broken because of GlideImage. See: https://github.com/bumptech/glide/issues/4977 | |||
There was a problem hiding this comment.
one thing we could do is wrap GlideImage in our own composable and use LocalInspectionMode to render e.g. red Box .. this way, the previews are working until the issue is resolved
There was a problem hiding this comment.
Good idea! Will create a wrapper for GlideImage
app/src/main/java/com/google/samples/apps/sunflower/compose/garden/GardenScreen.kt
Show resolved
Hide resolved
Change-Id: I6933ab8bdde68ad3adf1564ae7d4a59b0dac67be
|
|
||
| @Composable | ||
| private fun GardenScreen( | ||
| gardenPlants: List<PlantAndGardenPlantings>?, |
There was a problem hiding this comment.
this list won't be null anymore, right? because you pass initial = emptyList()
app/src/main/java/com/google/samples/apps/sunflower/compose/garden/GardenScreen.kt
Show resolved
Hide resolved
…rden/GardenScreen.kt Co-authored-by: Tomáš Mlynarič <mlynarict@gmail.com>
Change-Id: I782e23956891540e72ffeae1923fe7445c36172f
Resolves #816 #814
Fixes #773