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

Feature: Use jetpack navigation component #1088

Open
epicadk opened this issue Feb 20, 2021 · 19 comments · May be fixed by #1140
Open

Feature: Use jetpack navigation component #1088

epicadk opened this issue Feb 20, 2021 · 19 comments · May be fixed by #1140
Assignees
Labels
Category: Coding Changes to code base or refactored code that doesn't fix a bug. Type: Enhancement New feature or request.

Comments

@epicadk
Copy link
Member

epicadk commented Feb 20, 2021

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.

@epicadk epicadk added Category: Coding Changes to code base or refactored code that doesn't fix a bug. Type: Enhancement New feature or request. Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. labels Feb 20, 2021
@DhaneshShetty
Copy link
Contributor

Would like to work on this, once made available.

@epicadk
Copy link
Member Author

epicadk commented Feb 22, 2021

Would like to work on this, once made available.

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

@isabelcosta isabelcosta added Status: Available Issue was approved and available to claim or abandoned for over 3 days. Status: Possible Future Feature This needs to be discussed at a future time. and removed Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. labels Feb 23, 2021
@kartikeysaran
Copy link
Contributor

Can i work on this issue

@epicadk
Copy link
Member Author

epicadk commented Feb 24, 2021

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

@isabelcosta
Copy link
Member

@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
Copy link
Member

@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
Copy link
Member Author

epicadk commented Feb 25, 2021

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 isabelcosta removed the Status: Available Issue was approved and available to claim or abandoned for over 3 days. label Feb 28, 2021
@isabelcosta
Copy link
Member

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

@unc0ded
Copy link
Contributor

unc0ded commented May 23, 2021

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

@epicadk epicadk added Status: Available Issue was approved and available to claim or abandoned for over 3 days. and removed Status: Possible Future Feature This needs to be discussed at a future time. labels May 23, 2021
@epicadk
Copy link
Member Author

epicadk commented May 23, 2021

Marking available as discussed yesterday.

@epicadk
Copy link
Member Author

epicadk commented May 23, 2021

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

@epicadk
Copy link
Member Author

epicadk commented May 23, 2021

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
Copy link
Contributor

@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
Copy link
Member Author

epicadk commented May 23, 2021

@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
Copy link
Contributor

@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
Copy link
Member Author

epicadk commented May 24, 2021

@unc0ded assigning you

@epicadk epicadk removed the Status: Available Issue was approved and available to claim or abandoned for over 3 days. label May 25, 2021
@isabelcosta
Copy link
Member

@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
Copy link
Contributor

unc0ded commented May 29, 2021

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
Copy link
Member Author

epicadk commented May 29, 2021

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

@unc0ded unc0ded linked a pull request May 31, 2021 that will close this issue
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Coding Changes to code base or refactored code that doesn't fix a bug. Type: Enhancement New feature or request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants