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.

Sign up for GitHub

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

Jump to bottom

feat: added the external router for deep links #320

Merged
merged 1 commit into from
Jun 10, 2024
Merged

feat: added the external router for deep links #320

merged 1 commit into from
Jun 10, 2024

Conversation

dixidroid
Copy link
Contributor

@dixidroid dixidroid commented May 14, 2024 •
edited
Loading

Added ability to redirect from deep links/push notifications to different screens.

Screen_Recording_20240515_113916_Skype.mp4

@dixidroid dixidroid linked an issue May 14, 2024 that may be closed by this pull request
[Android] App navigational flow for deep links/push notifications #248 #202
Open
volodymyr-chekyrta
volodymyr-chekyrta requested changes May 15, 2024
View reviewed changes
Copy link
Contributor

@volodymyr-chekyrta volodymyr-chekyrta left a comment

Choose a reason for hiding this comment

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

Initial review.
Later, it will be added feedback after testing.
Overall, the approach is good, mostly code-style things.

app/src/main/java/org/openedx/app/AppExternalRouter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/openedx/app/AppExternalRouter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/openedx/app/AppExternalRouter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/openedx/app/AppExternalRouter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/openedx/app/AppViewModel.kt Outdated Show resolved Hide resolved
core/src/main/java/org/openedx/core/CourseTab.kt Outdated
Comment on lines 3 to 8
enum class CourseTab {
COURSE,
VIDEOS,
DISCUSSION,
DATES,
HANDOUTS
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this new dependency in the Core?
I'm not sure that Core should know about course screen UI. Maybe we can replace it just with bool flags?
let's discuss it.
@HamzaIsrar12 your opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with your point but I also see it as a deeplink-screen instead of a UI-screen. But to handle your concern, we can pass the tab value as ordinal or string and parse it on the other end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

core/src/main/java/org/openedx/core/HomeTab.kt Outdated
Comment on lines 3 to 7
enum class HomeTab {
DISCOVER,
DASHBOARD,
PROGRAMS,
PROFILE
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.
cc @HamzaIsrar12

core/src/main/java/org/openedx/core/CourseTab.kt Outdated Show resolved Hide resolved
core/src/main/java/org/openedx/core/HomeTab.kt Outdated Show resolved Hide resolved
@dixidroid dixidroid force-pushed the feat/links_router branch 4 times, most recently from 60d7b05 to 2031380 Compare May 17, 2024 11:56
@volodymyr-chekyrta
Copy link
Contributor

@HamzaIsrar12 please spare some time to review this PR 🙏

@HamzaIsrar12
Copy link
Contributor

I finally have the time to start the review. Thanks for your patience.

HamzaIsrar12
HamzaIsrar12 requested changes May 23, 2024
View reviewed changes
Copy link
Contributor

@HamzaIsrar12 HamzaIsrar12 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 the PR. ✨ It does the required job perfectly, but after some internal discussion, we've identified a few additional requirements.

Currently, the deep links are direct and lack backtrace functionality. We should maintain backtrace, similar to what we have implemented for iOS.

I also really appreciated the approach used in iOS with their DeepLinkManager, which works in the same way as our AnalyticsManager. It's a standalone class that integrates all the related SDKs and manages them effectively.

I'm attaching the iOS PR for inspiration, along with a video demo of their screen navigation.

Thanks again!

core/src/main/java/org/openedx/core/CourseTab.kt Outdated Show resolved Hide resolved
app/src/main/java/org/openedx/app/AppActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/org/openedx/app/AppExternalRouter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/openedx/app/AppExternalRouter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/openedx/app/AppExternalRouter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/openedx/app/AppExternalRouter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/openedx/app/AppRouter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/openedx/app/AppExternalRouter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/openedx/app/AppActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/org/openedx/app/AppExternalRouter.kt Outdated Show resolved Hide resolved
@dixidroid dixidroid force-pushed the feat/links_router branch 3 times, most recently from b7bc005 to 9210e92 Compare May 28, 2024 15:03
HamzaIsrar12
HamzaIsrar12 requested changes May 29, 2024
View reviewed changes
Copy link
Contributor

@HamzaIsrar12 HamzaIsrar12 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 addressing the comments. However, the feedback related to backtrace mentioned in the description of the last review still needs to be implemented.

Posting it again:

Thank you for the PR. ✨ It does the required job perfectly, but after some internal discussion, we've identified a few additional requirements.

Currently, the deep links are direct and lack backtrace functionality. We should maintain backtrace, similar to what we have implemented for iOS.

I also really appreciated the approach used in iOS with their DeepLinkManager, which works in the same way as our AnalyticsManager. It's a standalone class that integrates all the related SDKs and manages them effectively.

I'm attaching the iOS PR for inspiration, along with a video demo of their deeplink navigation.

Thanks again!

app/src/main/java/org/openedx/app/DeepLinkRouter.kt Outdated Show resolved Hide resolved
@dixidroid dixidroid force-pushed the feat/links_router branch 3 times, most recently from 110c56f to 3cc55bf Compare June 4, 2024 12:55
HamzaIsrar12
HamzaIsrar12 requested changes Jun 4, 2024
View reviewed changes
Copy link
Contributor

@HamzaIsrar12 HamzaIsrar12 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes Igor. It works perfectly. ✨

Minor nits only.

app/src/main/java/org/openedx/app/DeepLinkRouter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/openedx/app/MainFragment.kt Outdated Show resolved Hide resolved
app/src/main/java/org/openedx/app/DeepLinkRouter.kt Outdated Show resolved Hide resolved
core/src/main/java/org/openedx/core/CourseTab.kt Outdated
Comment on lines 3 to 8
enum class CourseTab {
COURSE,
VIDEOS,
DISCUSSION,
DATES,
HANDOUTS
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with your point but I also see it as a deeplink-screen instead of a UI-screen. But to handle your concern, we can pass the tab value as ordinal or string and parse it on the other end.

@dixidroid dixidroid force-pushed the feat/links_router branch 3 times, most recently from bdb7ba5 to aaaf5d8 Compare June 7, 2024 12:55
HamzaIsrar12
HamzaIsrar12 approved these changes Jun 7, 2024
View reviewed changes
Copy link
Contributor

@HamzaIsrar12 HamzaIsrar12 left a comment

Choose a reason for hiding this comment

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

Looks good to me 🏎️

@volodymyr-chekyrta volodymyr-chekyrta merged commit d845e5e into openedx:develop Jun 10, 2024
3 checks passed
@PavloNetrebchuk PavloNetrebchuk mentioned this pull request Nov 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@volodymyr-chekyrta volodymyr-chekyrta volodymyr-chekyrta approved these changes

@HamzaIsrar12 HamzaIsrar12 HamzaIsrar12 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Android] App navigational flow for deep links/push notifications #248
3 participants
@dixidroid @volodymyr-chekyrta @HamzaIsrar12