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

Migrated from navigating with parameters to type-safe navigation #162

Merged

Conversation

muhammadimran021
Copy link
Contributor

Description

Migrated from navigating with parameters to type-safe navigation

Type of change

Pull Request checklist

@starry-shivam
Copy link
Member

Wow! Thanks a lot! It was on my to-do list, but I hadn't found the time (and was also feeling lazy) to do it, haha. I'll try to review it soon.

Copy link
Member

@starry-shivam starry-shivam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, the changes look good and seem to work well. However, as I can see, you've only migrated screens that took arguments via the navigation backstack, but not all the other regular screens, which are still using string-based routes. I would like to migrate them as well and eliminate string-based routes completely. This will make the code much more consistent with the new type-safe navigation design and also make it more understandable for other contributors, like you, since we won't be mixing two different types of navigation APIs when one could handle everything in a better way.

@muhammadimran021
Copy link
Contributor Author

Hi, the changes look good and seem to work well. However, as I can see, you've only migrated screens that took arguments via the navigation backstack, but not all the other regular screens, which are still using string-based routes. I would like to migrate them as well and eliminate string-based routes completely. This will make the code much more consistent with the new type-safe navigation design and also make it more understandable for other contributors, like you, since we won't be mixing two different types of navigation APIs when one could handle everything in a better way.

Love to hear your feedback regarding this i'll change the whole screens to the new navigation api.

Handled multiple times screen navigation on tap of Home from drawer
@muhammadimran021
Copy link
Contributor Author

@starry-shivam Hey man just updated below points in this PR
Completely migrated to type safe navigation
Handled multiple times screen navigation on tap of Home from drawer

Copy link
Member

@starry-shivam starry-shivam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your efforts! it looks much better now. However, I think there are a few things to address before we merge this. I've left comments on the relevant parts.

app/build.gradle Outdated Show resolved Hide resolved
… checking at compile time.

Removed Gradle migration to type-safe navigation comment line.

Added back Header Comment in NormalScreens file.
Copy link
Member

@starry-shivam starry-shivam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great now, merging. Thanks for your contribution!

@starry-shivam starry-shivam merged commit 7f662bc into Pool-Of-Tears:main Oct 15, 2024
1 check passed
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 this pull request may close these issues.

2 participants