search

anitab-org / mentorship-android

Mentorship System is an application that matches women in tech to mentor each other, on career development, through 1:1 relations during a certain period of time. This is the Android application of this project.
https://github.com/anitab-org/mentorship-android/raw/apk/app-debug.apk
GNU General Public License v3.0
165 stars 379 forks source link

Feature: Use jetpack navigation component #1088

Open epicadk opened 3 years ago

epicadk commented 3 years ago

Is your feature request related to a problem? Please describe.

Currently we have a custom implementation for navigation.

Describe the solution you'd like

Use jetpack Navigation component. Also initialize the viewmodels from the NavController backstack.

Additional context

It is a very well documented library and using it would also reduce bugs due to navigation in our code.

DhaneshShetty commented 3 years ago

Would like to work on this, once made available.

epicadk commented 3 years ago

Would like to work on this, once made available.

Not sure if you can claim issues before they are made available.

kartikeysaran commented 3 years ago

Can i work on this issue

epicadk commented 3 years ago

@isabelcosta since it's a possible future feature can it be assigned now or should we wait?

isabelcosta commented 3 years ago

@vj-codes what was your thought behind possible future feature? All I know is that it isn't a priority, so it can be worked on now, but in terms of reviews, it will have less priority over other tasks. So if we assign, we should assign first to @DhaneshShetty

vj-codes commented 3 years ago

@isabelcosta what I meant was the navigation currently is alright for the MVP, we do have some features like forgot password, feedback email, resend email for verification, upload profile, improving ui, passing the tests that are necessary as compared to jetpack navigation. I'm not saying this can't be done now but as a user I wouldn't have a prblm with the current navigation but it'll be a huge impact if there's no forgot password feature in the app.

Another reason is the PR for this if merged will change the code structure a lot causing merge conflicts in all other PRs which were ready to merge and if we think about creating a PR and not reviewing now due to priority, but if there's some changes required weeks later I can't guarantee that the contributor assigned is available to solve them which will lead to closing it anyway. These are just my views, your call at the end :)

epicadk commented 3 years ago

I agree with @vj-codes although I do want to point out that if we want to add more tests to android then jetpack navigation is easier to test.

isabelcosta commented 3 years ago

agree with your points, let's leave this for now and focus on bigger issues :) cc @vj-codes @epicadk

unc0ded commented 3 years ago

Hi, is this available now? I'd like to work on it if possible.

epicadk commented 3 years ago

Marking available as discussed yesterday.

epicadk commented 3 years ago

@DhaneshShetty @kartikeysaran would you like to work on this?

epicadk commented 3 years ago

Hi, is this available now? I'd like to work on it if possible.

if they don't respond I'll assign this to you.

CodeChamp-SS commented 3 years ago

@DhaneshShetty @CodeChamp-SS would you like to work on this?

@epicadk my schedule is packed up right now, I won't get much time to fix this, feel free to assign @DhaneshShetty or @unc0ded :)

epicadk commented 3 years ago

@DhaneshShetty @CodeChamp-SS would you like to work on this?

@epicadk my schedule is packed up right now, I won't get much time to fix this, feel free to assign @DhaneshShetty or @unc0ded :)

Oops seems like I tagged you by mistake sorry.

CodeChamp-SS commented 3 years ago

@DhaneshShetty @CodeChamp-SS would you like to work on this?

@epicadk my schedule is packed up right now, I won't get much time to fix this, feel free to assign @DhaneshShetty or @unc0ded :)

Oops seems like I tagged you by mistake sorry.

it's fine, no problem 🙂

epicadk commented 3 years ago

@unc0ded assigning you

isabelcosta commented 3 years ago

@unc0ded , we just discussed in the weekly meeting and @epicadk brought up this issue. I think you could start working on this with regards to the bottom navigation.

I still don't know what this entails, but would be good to see a PR up (in case you want to try this out), and see how complex this is, and if people contributing to this repository could learn and adjust to starting using this feature.

unc0ded commented 3 years ago

Hi, I have started working on it, one of the main challenges to implementing this fully is the sheer number of activities (that could possibly be better suited as fragments). Navigation component works better in the single activity - multiple fragment architecture that google is also trying to push. Maybe converting some of the activities to fragments can be considered as a separate issue, to improve the architecture.

epicadk commented 3 years ago

Hi, I have started working on it, one of the main challenges to implementing this fully is the sheer number of activities (that could possibly be better suited as fragments). Navigation component works better in the single activity - multiple fragment architecture that google is also trying to push. Maybe converting some of the activities to fragments can be considered as a separate issue, to improve the architecture.

Yes this is what we had discussed in the open session as well. We can create separate issues