-
Notifications
You must be signed in to change notification settings - Fork 51
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
Swapped navigation to Jetpack Compose Destinations #184
Swapped navigation to Jetpack Compose Destinations #184
Conversation
…ibrary to Jetpack Compose Destinations
Hi, Thank you for your contribution. Looks like you dedicated your time entirely for this. I will for sure acknowledge your contribution at my social profiles. Also I will review this PR soon. Until then please run spotless apply command to clean ups wildcard imports. |
Thank you. I've some personal pending work to do. I'll look into the spotless evening. |
screen/ui-article-detail/src/main/java/kasem/sm/ui_detail/DetailVM.kt
Outdated
Show resolved
Hide resolved
Btw I guess UI Tests needs to be rewritten na? |
I think, Yes, but now it's easy to write them. I mean we have abstraction there. |
Yup. Got it. I would do that so I can also get familiar with the Compose Destination library. I would merge this PR soon! |
screen/ui-article-detail/src/main/java/kasem/sm/ui_detail/DetailVM.kt
Outdated
Show resolved
Hide resolved
@rvenky125 Found an issue. screen-20220730-065545.2.mp4I'm debugging this issue too. Let me know when you find any fix. |
Oh, I'll check that now |
I found the issue. Please refer to the pending review at top :) . |
Yeah, I fixed that and committed. Sorry, it's really a stupid thing I did. |
No issues. You did a lot of refactoring in past few days. Great and Thank You again for this hard work. |
Also there are lot of code violation when you run |
Is it? I've already run that. I just run again. I'm getting BUILD SUCCESSFUL. Can you pls check again with the latest commit? |
I just realized that I've changed the way of using the backhander. Do you really need it as a dependency? If yes, I'll commit by implementing it. |
I've implemented that backhander as a dep. Let me if you wanted to be in the code. |
I'm thankful to you also. I've learnt how to implement compose destinations in Multi Module arch. |
if You got Build Success, then check if there is any uncommitted files. |
Can you please share the file or line number? |
Also replace the content of the file inside of |
It's the HomeScreen composable. Where in previous you have backhander as function to it. But I've replaced it with direct import of BackHandler. Once check the changes in HomeScreen composable, You'll get an idea |
Thank you @rvenky125 for your valuable contribution to this repository. |
It's my pleasure. It's my first contribution and you liked it. It's encouraging me to do more contributions. |
Definitely. Also, Open source contributions can be added on your portfolio/resume as well and believe me, it helps a lot. |
I've downgraded the compose version to 1.2 in order to use the Jetpack Compose Destinations library. I've tried to make it as much scalable as possible. Let me know if I had violated the project structure or any other clean code. I'll correct and will create another pull request.