-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat: Pre-Login Experience enhancement by integrating webview discovery #215
feat: Pre-Login Experience enhancement by integrating webview discovery #215
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial Code Review; however, still need to perform an exhaustive manual review with all use cases.
Please use named parameters for the router with infoType
or use default values, the plain null
value is a bit confusing. (+1 for default value)
discovery/src/main/java/org/openedx/discovery/presentation/DiscoveryNavigator.kt
Outdated
Show resolved
Hide resolved
discovery/src/main/java/org/openedx/discovery/presentation/WebViewDiscoveryFragment.kt
Outdated
Show resolved
Hide resolved
discovery/src/main/java/org/openedx/discovery/presentation/WebViewDiscoveryFragment.kt
Outdated
Show resolved
Hide resolved
auth/src/main/java/org/openedx/auth/presentation/signup/SignUpFragment.kt
Outdated
Show resolved
Hide resolved
course/src/main/java/org/openedx/course/presentation/info/CourseInfoFragment.kt
Outdated
Show resolved
Hide resolved
discovery/src/main/java/org/openedx/discovery/presentation/WebViewDiscoveryViewModel.kt
Outdated
Show resolved
Hide resolved
course/src/main/java/org/openedx/course/presentation/info/CourseInfoViewModel.kt
Outdated
Show resolved
Hide resolved
course/src/main/java/org/openedx/course/presentation/info/CourseInfoViewModel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test cases are failing ...
auth/src/main/java/org/openedx/auth/presentation/logistration/LogistrationFragment.kt
Show resolved
Hide resolved
auth/src/main/java/org/openedx/auth/presentation/signup/SignUpFragment.kt
Show resolved
Hide resolved
course/src/main/java/org/openedx/course/presentation/info/CourseInfoFragment.kt
Outdated
Show resolved
Hide resolved
course/src/main/java/org/openedx/course/presentation/info/CourseInfoFragment.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ✨ Great work!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome job! Just a little question
requireArguments().apply { | ||
this.getString(ARG_COURSE_ID, null)?.apply { | ||
router.navigateToCourseDetail(parentFragmentManager, this) | ||
requireArguments().run { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we changed apply to run here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the oversight. Since we are not returning any value, we can proceed with using apply
, as we are updating the bundle properties after processing them.
@k1rill thorughs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, please resolve conflicts
- Integrate search to align with configured discovery settings. - “Explore All Courses" redirects based on the feature discovery flag. - Auth Panel (SignIn, Register) remains visible during pre-login webview discovery. - Ensure Sign In & Enroll mirror Native Discovery & Market App functionality. - Clear edit text after submit Logistration. - Fix test cases for View Models. LEARNER-9798
43a5b9c
118a3e3
to
43a5b9c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🏎️
Description
Screen_recording_20240201_162216.webm