Compare menus #148

oliviaxjiang · 2024-09-27T04:37:07Z

Problems Addressed

1. Issue #142: Scrolling Header

✅ Resolved by refactoring the EateryDetailsStickerHeader and using it in CompareMenusScreen

2. Issue #141: Sheet Stays Open

✅ Fixed by adding an additional close line to OnCompareMenusClick. Did not used NavController or LaunchedEffect

3. Issue #140: Formatting Stuff

🟡 Fixed except middle format issue Figma design is rectangular borders, so idk how to fix

4. Issue #138: Row Clickable

✅ Fixed this issue per the outlined requirements. The app behavior should now match the expected outcome described in the issue.

5. Issue #139 : eatery name bottom padding

✅ Fixed added padding according to figma design

6. Issue #137: Not Done

🚫 Not Addressed: IDK what filter cuts off means.

View Model Logic Description

CompareMenusBotSheet needs to manage its own states (adding/removing selected eateries/filters) so it gets it's own viewModel (CompareMenusBotViewModel). CompareMenusScreen also needs state management, so it also gets a view model (CompareMenusViewModel). No specific viewmodel logic resides in HomeViewModel or EateryDetailsViewModel as they are all delegated to the two new view models.

Potential places for change (Would love advice here!)

List called eateryIds currently gets sent to CompareMenusScreen and CompareMenusScreen has to call a function to pass that information into the viewmodel, may want to used sharedStateHandle and give it directly to CompareMenusViewModel? (I tried looking up on StackOverFlow on how to share states between two screens that is connected via NavController and someone said that the best practice would be sharedViewModel (?))

in compare menus bot sheet

zachseidner1

This is a great first draft! I like your decision to give the compare menus bottom sheet a ViewModel and injecting it via hiltViewModel(). After seeing all the filtering logic it definitely makes sense. There are a bit of comments that you need to work through, anything, so much so that anything I labeled with "nit" don't feel pressure to fix. But I really appreciate the work you've put into implementing this and I hope you've learned more about Android architecture!

zachseidner1 · 2024-09-27T18:51:19Z

...ain/java/com/cornellappdev/android/eatery/ui/components/comparemenus/CompareMenusBotSheet.kt

+@Composable
+fun CompareMenusBotSheet(
+    onDismiss: () -> Unit,
+    onCompareMenusClick: (selectedEateries : List<Eatery>) -> Unit,


Since you map this to a list of IDs anyway, this should be (selectedEateryIds: List<Int>) -> Unit.

zachseidner1 · 2024-09-27T18:59:38Z

...ain/java/com/cornellappdev/android/eatery/ui/components/comparemenus/CompareMenusBotSheet.kt

+                ) {
+                    LazyColumn {
+                        items(eateries) { eatery ->
+                            Row(


Can you abstract this component into a private composable like EateryTextRow, that takes an eatery name as a parameter? I think it could make this file a bit more readable.

Ya good point! There's a lot of indentation here.

I think it's generally good to take out the header, content, and bottom portions into separate composables for readability.

Veronica discovered this feature where you just highlight a bunch of your function and hit Extract that pops up and it does this all for you. I don't know how accurate/good it is though, but perhaps try it out!

zachseidner1 · 2024-09-27T19:00:23Z

...src/main/java/com/cornellappdev/android/eatery/ui/components/comparemenus/CompareMenusFAB.kt

+    ) {
+        Icon(
+            painter = painterResource(id = R.drawable.ic_compare_menus),
+            "Floating action button.",


Nit: could you label this with "Compare menus" for accessibility

zachseidner1 · 2024-09-27T19:04:39Z

app/src/main/java/com/cornellappdev/android/eatery/ui/components/general/FilterRow.kt

@@ -110,6 +110,100 @@ fun FilterRow(
    }
 }

+@Composable
+fun CompareFilterRow(


Nit: all top level Composable functions should have previews associated with them

zachseidner1 · 2024-09-27T19:06:40Z

app/src/main/java/com/cornellappdev/android/eatery/ui/components/settings/AppIconBottomSheet.kt


+@OptIn(ExperimentalPagerApi::class, ExperimentalFoundationApi::class)


Nit: remove ExperimentalPagerApi::class since it is unused

zachseidner1 · 2024-09-27T19:51:32Z

app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/CompareMenusBotViewModel.kt

+                filters = currentState.filters + listOf(filter)
+            )
+        }
+        updateFilteredEateries()


We shouldn't force the implementer to remember to call this updateFilteredEateries method. This is what flows are for! We can combine a favorites flow, filters flow, and eatery flow and allow this update to happen automatically whenever the filters flow changes. This isn't too big of a deal though, and I know I've already left a lot of comments, so you don't have to address this. It probably would be fun to write though.

zachseidner1 · 2024-09-27T19:52:25Z

app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/CompareMenusBotViewModel.kt

+        }
+
+        if (filters.contains(Filter.FAVORITES)) {
+            passesFilter = favorites[eatery.id] == true


If it fails the under 10 filter but passes the favorites filter, then wouldn't this code make it so it still shows?

zachseidner1 · 2024-09-27T19:54:46Z

app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/CompareMenusBotViewModel.kt

+        }
+    }
+
+    private fun passesFilter(


Nit: I think a more clean implementation would be to create a map of filters to selector functions, such that each Filter maps to some (Eatery) -> Boolean expression. Then this function would just return filters.all { it.mapToFilter()(eatery) }

zachseidner1 · 2024-09-27T19:55:06Z

app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/CompareMenusBotViewModel.kt

Nice job with this overall!

zachseidner1 · 2024-09-27T20:00:56Z

app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/HomeViewModel.kt

Why are we changing this file? All the state for CompareMenusBottomSheet should be stored in the ViewModel that corresponds to it right?

Oh yeah good point... what are all these functions for?

thisjustin123

I'll defer the approval of this PR to zach since his comments are incredibly comprehensive. But I just left some tiny additional comments.

Thanks for the review Zach. U da man.

...ain/java/com/cornellappdev/android/eatery/ui/components/comparemenus/CompareMenusBotSheet.kt

thisjustin123 · 2024-09-28T02:44:22Z

...ain/java/com/cornellappdev/android/eatery/ui/components/comparemenus/CompareMenusBotSheet.kt

+                ) {
+                    LazyColumn {
+                        items(eateries) { eatery ->
+                            Row(


Ya good point! There's a lot of indentation here.

thisjustin123 · 2024-09-28T02:45:12Z

...ain/java/com/cornellappdev/android/eatery/ui/components/comparemenus/CompareMenusBotSheet.kt

+                ) {
+                    LazyColumn {
+                        items(eateries) { eatery ->
+                            Row(


I think it's generally good to take out the header, content, and bottom portions into separate composables for readability.

Veronica discovered this feature where you just highlight a bunch of your function and hit Extract that pops up and it does this all for you. I don't know how accurate/good it is though, but perhaps try it out!

thisjustin123 · 2024-09-28T02:45:34Z

...ain/java/com/cornellappdev/android/eatery/ui/components/details/EateryDetailsStickyHeader.kt

-    eateryDetailViewModel: EateryDetailViewModel = hiltViewModel(),
-    onItemClick: (Int) -> Unit
+    startIndex : Int,
+    onItemClick: (Int) -> Unit,


Nice, love the removal of viewmodel.

thisjustin123 · 2024-09-28T02:46:49Z

app/src/main/java/com/cornellappdev/android/eatery/ui/components/settings/AppIconBottomSheet.kt

+        val pagerState = androidx.compose.foundation.pager.rememberPagerState()
+        androidx.compose.foundation.pager.HorizontalPager(pageCount = 2, state = pagerState, modifier = Modifier.offset(x = -8.dp)){
+            Column{
+                when(it){


I smell unformatted file >:)

thisjustin123 · 2024-09-28T02:47:28Z

app/src/main/java/com/cornellappdev/android/eatery/ui/navigation/MainTabbedNavigation.kt

+    val showBottomBar = rememberSaveable { mutableStateOf(false)
+    }


Strange formatting!

thisjustin123 · 2024-09-28T02:47:57Z

app/src/main/java/com/cornellappdev/android/eatery/ui/navigation/MainTabbedNavigation.kt

+        composable(
+            route = "comparemenus/{eateryIds}",
+            arguments = listOf(navArgument("eateryIds") { type = NavType.StringType }),
+            enterTransition = { fadeIn(animationSpec = tween(durationMillis = 500)) },
+            exitTransition = { fadeOut(animationSpec = tween(durationMillis = 500)) }
+        ) { backStackEntry ->
+            backStackEntry.arguments?.getString("eateryIds")?.split(",")?.map { it.toInt() }?.let { eateryIds ->
+                CompareMenusScreen(
+                    eateryIds = eateryIds,
+                    onEateryClick = {
+                        FirstTimeShown.firstTimeShown = false
+                        navController.navigate("${Routes.EATERY_DETAIL.route}/${it.id}")
+                    }
+                )
+            }
+        }


lovely stuff, this passing of IDs is exactly what we need!

thisjustin123 · 2024-09-28T02:48:39Z

app/src/main/java/com/cornellappdev/android/eatery/ui/screens/CompareMenusScreen.kt

Oh god yeah let's split this up.

app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/CompareMenusBotViewModel.kt

thisjustin123 · 2024-09-28T02:51:33Z

app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/HomeViewModel.kt

Oh yeah good point... what are all these functions for?

- onCompareMenusClick now requires List<Int> instead of List<Eatery> so navigation doesn't take care of the id mapping - extracted lazy list in CM bottom sheet

- added compare menus key word to CM FAB content description - only one flow (UiState) in compare menus bot sheet view model - private composable in compare menus screen

zachseidner1

W

# Conflicts: # app/src/main/java/com/cornellappdev/android/eatery/ui/screens/EateryDetailScreen.kt # app/src/main/java/com/cornellappdev/android/eatery/ui/screens/HomeScreen.kt

oliviaxjiang and others added 16 commits April 12, 2024 00:23

CM FAB and basic ui structure for Bottom sheet

134051d

allowed checkmarks to be selected + added navigation to screen

93328c7

bot sheet button color

3c02335

bottom sheet filter functionality

7fc7e16

navigation passes parameter (next step: viewmodel)

278a168

some progress on displaying items.

b954171

Update CompareMenusScreen.kt

15f896a

view eatery details

8442cea

Changed Compare Menus Icon, added Selected Filter

2f187b6

no eatery menu

387637f

Merge branch 'main' into compare-menus

2155809

Add animations for FAB and simplify showing logic

f8500bb

compare menus sticker header

4a8e2d6

both row and icon button clickable

d99096b

in compare menus bot sheet

compare menus view model refactor

6541231

reformat

c308810

oliviaxjiang requested review from thisjustin123 and zachseidner1 September 27, 2024 04:37

zachseidner1 requested changes Sep 27, 2024

View reviewed changes

thisjustin123 reviewed Sep 28, 2024

View reviewed changes

oliviaxjiang added 8 commits September 28, 2024 10:49

addressing PR comments

90379e0

- onCompareMenusClick now requires List<Int> instead of List<Eatery> so navigation doesn't take care of the id mapping - extracted lazy list in CM bottom sheet

address PR comment pt 2

201ace2

- added compare menus key word to CM FAB content description - only one flow (UiState) in compare menus bot sheet view model - private composable in compare menus screen

Update CompareMenusScreen.kt

2d4740e

Update CompareMenusScreen.kt

a91b9bd

removed unused functions in homeviewmodel

878d5b6

reformat code

d5106bb

fix filter logic

4f2ad53

fixed favorite filter logic

9acb865

oliviaxjiang requested a review from zachseidner1 October 2, 2024 21:06

Refactor

f67c277

zachseidner1 approved these changes Oct 2, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into compare-menus

bafe771

# Conflicts: # app/src/main/java/com/cornellappdev/android/eatery/ui/screens/EateryDetailScreen.kt # app/src/main/java/com/cornellappdev/android/eatery/ui/screens/HomeScreen.kt

zachseidner1 force-pushed the compare-menus branch from bafe771 to f67c277 Compare October 2, 2024 23:01

resolved conflicts

ce85db8

oliviaxjiang merged commit 78be1e7 into main Oct 3, 2024

thisjustin123 deleted the compare-menus branch October 23, 2024 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compare menus #148

Compare menus #148

oliviaxjiang commented Sep 27, 2024

zachseidner1 left a comment

zachseidner1 Sep 27, 2024

zachseidner1 Sep 27, 2024

thisjustin123 Sep 28, 2024

thisjustin123 Sep 28, 2024

zachseidner1 Sep 27, 2024

zachseidner1 Sep 27, 2024

zachseidner1 Sep 27, 2024

zachseidner1 Sep 27, 2024

zachseidner1 Sep 27, 2024

zachseidner1 Sep 27, 2024

zachseidner1 Sep 27, 2024

zachseidner1 Sep 27, 2024

thisjustin123 Sep 28, 2024

thisjustin123 left a comment

thisjustin123 Sep 28, 2024

thisjustin123 Sep 28, 2024

thisjustin123 Sep 28, 2024

thisjustin123 Sep 28, 2024

thisjustin123 Sep 28, 2024

thisjustin123 Sep 28, 2024

thisjustin123 Sep 28, 2024

thisjustin123 Sep 28, 2024

zachseidner1 left a comment


		@OptIn(ExperimentalPagerApi::class, ExperimentalFoundationApi::class)

		val showBottomBar = rememberSaveable { mutableStateOf(false)
		}

Compare menus #148

Compare menus #148

Conversation

oliviaxjiang commented Sep 27, 2024

Problems Addressed

1. Issue #142: Scrolling Header

2. Issue #141: Sheet Stays Open

3. Issue #140: Formatting Stuff

4. Issue #138: Row Clickable

5. Issue #139 : eatery name bottom padding

6. Issue #137: Not Done

View Model Logic Description

Potential places for change (Would love advice here!)

zachseidner1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisjustin123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachseidner1 left a comment

Choose a reason for hiding this comment