Move Add a meal to a new activity #93
Conversation
roger-tan
requested review from
fmontesino,
Gmiller290488,
kirstybutler and
dsmith42
| super.onCreateOptionsMenu(menu, inflater) | ||
|
|
||
| inflater.inflate(R.menu.find_meal_container_menu, menu) | ||
| } |
There was a problem hiding this comment.
I think we're having the adding of a meal within a second tab of the bottom nav rather than this
There was a problem hiding this comment.
In the iOS side, it was a new modal from the map.
| @@ -0,0 +1,54 @@ | |||
| package com.kcc.kmmhackathon.androidHackathonApp.view | |||
There was a problem hiding this comment.
I'm confused as to why you've created this file - please could you give some more context?
There was a problem hiding this comment.
I created this file because I don't think that the find MapsFragment should know when to create a new add meal activity
There was a problem hiding this comment.
Good point! But following that logic, the FindMeal also shouldn't know about creating new meals and creating a new container just for that seems a bit overkill.
I'd add this logic in the Activity holding the Fragments and BottomNavigation, maybe in a FloatingActionButton.
Either way, FAB or action in the Toolbar, the action belongs to the Activity, and then you have 3 independent Fragments to navigate to.
| <item android:id="@+id/findMealFragment" | ||
| android:icon="@drawable/ic_baseline_list_24" | ||
| android:title="@string/title_switch_to_list"/> | ||
|
|
||
| <item | ||
| android:id="@+id/settingsFragment" | ||
| android:icon="@drawable/ic_baseline_settings_24" |
There was a problem hiding this comment.
This is going to conflict with Fidel's PR with the refactoring of the FindMealFragment, no?
There was a problem hiding this comment.
Pretty sure that's going to be conflicted.
| android:title="Add" | ||
| android:orderInCategory="100" | ||
| yourapp:showAsAction="always" /> | ||
| </menu> |
There was a problem hiding this comment.
Again I think this should stay as a tab :)
| @@ -23,7 +23,7 @@ | |||
| <string name="hint_password">Enter Password</string> | |||
|
|
|||
| //Headers | |||
| <string name="add_meal_activity_header">This is the add meal activity screen</string> | |||
There was a problem hiding this comment.
Do we need it as an Activity?
There was a problem hiding this comment.
If we want to have the toolbar yes!
| val latLng = LatLng(it.latitude, it.longitude) | ||
| updateUserLocation(latLng) | ||
| updateMeals(latLng) | ||
| if (it != null) { |
There was a problem hiding this comment.
Is this null check necessary? Would we get to startUpdatingLocation if there was no current location?
There was a problem hiding this comment.
It was crashing on my side without checking if it's null.
There was a problem hiding this comment.
Also, just as a minor thing, when creating a PR please could you link it to an issue within the Projects? And also add a label as Android so that it's easy to keep track of which platform this is for? It will help make it easier for new people joining the project to understand more of what's going on and also for us to view the board in a more organised way 🙌 😃
What I've been done :
feature-add-mealandfeature-find-mealshould not know each other. Let me know about this.Quick preview:
