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

Try out Compose Destinations library #96

Closed
kasem-sm opened this issue Apr 22, 2022 · 12 comments · Fixed by #184
Closed

Try out Compose Destinations library #96

kasem-sm opened this issue Apr 22, 2022 · 12 comments · Fixed by #184

Comments

@kasem-sm
Copy link
Owner

Ref - https://github.com/raamcosta/compose-destinations

@rvenky125
Copy link
Contributor

I'm willing to try this out, if you haven't started it yet

@kasem-sm
Copy link
Owner Author

I'm willing to try this out, if you haven't started it yet

yes, you can try. Thank You! 😃

@rvenky125
Copy link
Contributor

rvenky125 commented Jul 24, 2022

I did a little bit workaround on it. But the library requires compose version of 1.1 or 1.2

@kasem-sm
Copy link
Owner Author

I see. If the library provides significant benefits over the official compose navigation library, I think we should downgrade the compose version.

@rvenky125
Copy link
Contributor

I agree, let me know if I can help you in downgrading the compose version.

@kasem-sm
Copy link
Owner Author

Please create a fork and downgrade the compose version in order to try the Compose Destination library. Looking forward for your code contribution to this repo.

@rvenky125
Copy link
Contributor

Okay, I'm very excited for my first contribution. It'll take time because of my tight schedule. But I'll try to do it every day. Thank you.

@kasem-sm
Copy link
Owner Author

No issues. Don't hesitate to connect with me if you need any help with the implementation. Good Luck!

@rvenky125
Copy link
Contributor

Hello @kasem-sm. You have passed functions for the navigation among the screens. It'll be easier if we declare those functions in the specific screen bypassing the nav controller. Is there any strong reason to not doing like that? Now I'm willing to do that. Are ok with that?

@kasem-sm
Copy link
Owner Author

Hi. I wanted to have the project scalable. I guess the compose destination library has its own wrapper around nav controller. Imagine replacing the parameters at 10 or more different screen just because you swapped out a library. In future if you shifted to KMM and want to use Decompose library for navigation or any other, it would be just easy to change things at one file or one module rather than 10 different modules.
This is my personal opinion and I may not be right. If you think there is some other way, please let me know :)

@rvenky125
Copy link
Contributor

rvenky125 commented Jul 27, 2022

Yeah, you are right. I can achieve your requirement by providing dependencies through the NavHost to each module. But still using the route(string) for navigation is not sense. So instead of using the route as a param for those lambda functions, we can pass an object of the destination sealed class you have created. Instead of having the route in the sealed class we can use the auto generated Direction object. Now we can define our navigation functions in the app module. If we are going to change to any navigation library we will just change the sealed class(Destination) and the navigation functions in NavHost.

@kasem-sm
Copy link
Owner Author

Sounds like it's a good plan. You can proceed with it as this project is open to new ideas.

@kasem-sm kasem-sm linked a pull request Jul 30, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants