-
Notifications
You must be signed in to change notification settings - Fork 25
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: [FC-0047] Improved Dashboard Level Navigation #308
feat: [FC-0047] Improved Dashboard Level Navigation #308
Conversation
…dpoint for UserCourses screen.
…rCoursesScreen onClick methods
# Conflicts: # core/build.gradle # core/src/main/java/org/openedx/core/ui/ComposeExtensions.kt # course/src/main/java/org/openedx/course/presentation/container/CourseContainerAdapter.kt # course/src/main/java/org/openedx/course/presentation/container/CourseContainerFragment.kt # course/src/main/java/org/openedx/course/presentation/container/CourseContainerViewModel.kt # dashboard/src/main/java/org/openedx/dashboard/presentation/DashboardRouter.kt # discovery/src/main/java/org/openedx/discovery/presentation/DiscoveryRouter.kt
Thanks for the pull request, @PavloNetrebchuk! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
5fb01ff
to
5e04ae6
Compare
24dbbe4
to
e097f31
Compare
Hi @PavloNetrebchuk, thanks for these changes. They look really good overall. I've noted all the minor visual issues I could find below. This is a huge usability/experience win regardless of these issues, so if any are difficult to resolve, most of them could be saved for a future cleanup. iOS:
Android:
Both iOS and Android:
|
bae47fe
to
c03832f
Compare
Hello, @sdaitzman |
# Conflicts: # course/src/main/java/org/openedx/course/presentation/container/CourseContainerFragment.kt # course/src/main/java/org/openedx/course/presentation/container/CourseContainerViewModel.kt # course/src/main/java/org/openedx/course/presentation/outline/CourseOutlineViewModel.kt # course/src/main/java/org/openedx/course/presentation/unit/container/CourseUnitContainerViewModel.kt
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.
Great job! Just some questions from my side
core/src/main/java/org/openedx/core/data/model/CourseAssignments.kt
Outdated
Show resolved
Hide resolved
core/src/main/java/org/openedx/core/data/model/CourseDateBlock.kt
Outdated
Show resolved
Hide resolved
core/src/main/java/org/openedx/core/data/model/CourseDateBlock.kt
Outdated
Show resolved
Hide resolved
core/src/main/java/org/openedx/core/data/model/EnrolledCourse.kt
Outdated
Show resolved
Hide resolved
dashboard/src/main/java/org/openedx/courses/presentation/PrimaryCourseFragment.kt
Outdated
Show resolved
Hide resolved
dashboard/src/main/java/org/openedx/dashboard/domain/CourseStatusFilter.kt
Outdated
Show resolved
Hide resolved
dashboard/src/main/java/org/openedx/dashboard/domain/interactor/DashboardInteractor.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.
Config: IMO, we can use more appropriate Dashboard
types
- List
- Grid (instead of
primary_course
)
we can update the files names accordingly.
1- The course not yet started but the progress is more than 0% + 2 past due assignment
badge is visible.
2- Programs are not loading (might be sandbox issue) but no proper UI handling i.e, loader or error page etc.
3- tapping on Expired
doesn't scroll horizontally
Screen.Recording.2024-05-20.at.9.22.35.PM.mov
4- If the Course is not started yet, the App is unable to show a proper message.
Great work on this PR!
1- Can we add trailing commas to the parameter lists for better readability and easier future modifications?
2- We should follow the implementation conventions for UI Util classes. Previously, we were defining UIUtil classes with ABCView, but in the current implementation, it is ABCScreen. Can we update it accordingly?
3- Can we update the course card once the user did any assignment or change in course progress instead of doing scrollDownToRefresh?
Thanks!"
core/src/main/res/values/colors.xml
Outdated
@@ -3,4 +3,6 @@ | |||
<color name="background">#FFFFFF</color> | |||
<color name="primary">#3C68FF</color> | |||
<color name="splash">#517BFE</color> | |||
<color name="checked_tab_item">#3C68FF</color> | |||
<color name="unchecked_tab_item">#97A5BB</color> | |||
</resources> |
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.
Please add a line at EOF.
@@ -3,4 +3,6 @@ | |||
<color name="background">#FF19212F</color> | |||
<color name="primary">#5478F9</color> | |||
<color name="splash">#19212F</color> | |||
<color name="checked_tab_item">#879FF5</color> | |||
<color name="unchecked_tab_item">#8E9BAE</color> | |||
</resources> |
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.
Please add a line at EOF.
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.
Please auto-format this file.
val dir = context.externalCacheDir.toString() + File.separator + | ||
context.getString(org.openedx.core.R.string.app_name).replace(Regex("\\s"), "_") | ||
val file = File(dir) | ||
file.mkdirs() | ||
return file | ||
} | ||
|
||
inline fun < reified T> saveObjectToFile(obj: T, fileName: String = "${T::class.java.simpleName}.json") { |
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.
Requires auto-format.
null | ||
} | ||
} | ||
|
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.
Please remove extra line.
private val resourceManager: ResourceManager, | ||
private val discoveryNotifier: DiscoveryNotifier, | ||
private val analytics: DashboardAnalytics, | ||
val dashboardRouter: DashboardRouter |
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.
Can we make this private and move its methods in ViewModel from fragment.
@@ -0,0 +1,663 @@ | |||
package org.openedx.courses.presentation |
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.
Requires auto-format.
|
||
@Composable | ||
private fun AllEnrolledCoursesScreen( | ||
viewModel: AllEnrolledCoursesViewModel = koinViewModel(), |
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.
we can move it to a private member
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.
still pending
@@ -0,0 +1,663 @@ | |||
package org.openedx.courses.presentation | |||
|
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.
Can we make an UI class for Dashboard(Course cards, Headers, States etc) for better readability.
} | ||
|
||
is PrimaryCourseScreenAction.NavigateToDates -> { | ||
viewModel.dashboardRouter.navigateToCourseOutline( |
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.
can we make dashboardRouter private and move these method inside ViewModel?
b4a6967
to
9454c60
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.
Just last question left from my side
dashboard/src/main/java/org/openedx/courses/presentation/AllEnrolledCoursesViewModel.kt
Outdated
Show resolved
Hide resolved
fc95cfc
to
115a9c8
Compare
115a9c8
to
31b3843
Compare
# Conflicts: # app/src/main/res/layout/fragment_main.xml # course/src/main/java/org/openedx/course/presentation/ui/CourseUI.kt
8779a3c
to
7ef6b77
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.
some minor changes only.
} else { | ||
InDevelopmentFragment() | ||
val discoveryFragment = DiscoveryNavigator(viewModel.isDiscoveryTypeWebView).getDiscoveryFragment() | ||
val dashboardFragment = when (viewModel.dashboardType) { |
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.
Can we write a navigator for Dashboard same as we have for Discovery.
class DashboardNavigator(
private val dashboardType: DashboardType,
) {
fun getDashboardFragment(): Fragment {
return when (dashboardType) {
DashboardConfig.DashboardType.GALLERY -> LearnFragment()
else -> DashboardListFragment()
}
}
}
default_config/dev/config.yaml
Outdated
WEBVIEW: | ||
PROGRAM_URL: '' | ||
PROGRAM_DETAIL_URL_TEMPLATE: '' | ||
|
||
DASHBOARD: | ||
TYPE: 'primary_course' |
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.
We have updated the types to List vs Gallery
.
We need to update the config as well.
cc: @miankhalid @saeedbashir
|
||
fun getExternalAppDir(context: Context): File { | ||
fun getExternalAppDir(): File { |
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.
Still pending CourseSectionFragment:137
} | ||
|
||
override fun getProgramFragmentInstance(): Fragment { | ||
return ProgramFragment(myPrograms = true, isNestedFragment = true) |
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.
@PavloNetrebchuk please create a ticket for this and mentioned it for tracking.
|
||
@Composable | ||
private fun Header( | ||
viewModel: LearnViewModel = koinViewModel(), |
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.
we can make it a private method.
} | ||
} | ||
|
||
interface DashboardGalleryScreenAction { |
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.
Can we move it to a separate file?
courseId = enrolledCourse.course.id, | ||
courseTitle = enrolledCourse.course.name, | ||
enrollmentMode = enrolledCourse.mode, | ||
openTab = if (openDates) DATES_TAB else "", |
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.
we can define a enum class here for tabs and write it as
openTab = if (openDates) CourseTab.DATES.name else CourseTab.HOME.name
enum class CourseTab {
HOME, VIDEOS, DATES, DISCUSSIONS, MORE
}
private val dashboardRouter: DashboardRouter | ||
) : BaseViewModel() { | ||
|
||
companion object { |
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.
we can use an enumClass instead
|
||
@Composable | ||
fun DashboardGalleryView( | ||
viewModel: DashboardGalleryViewModel = koinViewModel(), |
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.
we can move it to private.
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!
# Conflicts: # dashboard/src/main/java/org/openedx/courses/presentation/AllEnrolledCoursesFragment.kt # dashboard/src/main/java/org/openedx/courses/presentation/DashboardGalleryViewModel.kt
During the development process, we identified the need to refactor the ProgramsFragment. |
# Conflicts: # core/src/main/java/org/openedx/core/ui/ComposeCommon.kt # core/src/main/res/values/strings.xml # course/src/main/java/org/openedx/course/presentation/container/CourseContainerViewModel.kt # course/src/main/java/org/openedx/course/presentation/dates/CourseDatesScreen.kt # course/src/main/java/org/openedx/course/presentation/dates/CourseDatesViewModel.kt # course/src/main/java/org/openedx/course/presentation/outline/CourseOutlineViewModel.kt # course/src/main/res/values/strings.xml # course/src/test/java/org/openedx/course/presentation/dates/CourseDatesViewModelTest.kt
@PavloNetrebchuk 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
* feat: Created Learn screen. Added course/program navigation. Added endpoint for UserCourses screen. * feat: Added primary course card * feat: Added start/resume course button * feat: Added alignment items * feat: Fix future assignment date, add courses list, add onSearch and onCourse clicks * feat: Add feature flag for enabling new/old dashboard screen, add UserCoursesScreen onClick methods * feat: Create AllEnrolledCoursesFragment. Add endpoint parameters * feat: AllEnrolledCoursesFragment UI * feat: Minor code refactoring, show cached data if no internet connection * feat: UserCourses screen data caching * feat: Dashboard * refactor: Dashboard type flag change, start course button change * feat: Added programs fragment to LearnFragment viewPager * feat: Empty states and settings button * fix: Number of courses * fix: Minor UI changes * fix: Fixes according to designer feedback * fix: Fixes after demo * refactor: Move CourseContainerTab * fix: Fixes according to PR feedback * fix: Fixes according to PR feedback * feat: added a patch from Omer Habib * fix: Fixes according to PR feedback
* feat: Course Home progress bar * feat: Collapsing course sections * feat: New download icons * feat: show CourseContainerFragment if COURSE_NESTED_LIST_ENABLED false * fix: course progress bar updating * feat: Renamed COURSE_NESTED_LIST_ENABLE feature flag * feat: Course home. Moved certificate access. * chore: enhance app theme capability for prod edX theme/branding (#262) chore: enhance app theme capability for prod edX theme/branding - Integrate Program config updates - theming/branding code improvements for light and dark modes - Force dark mode for the WebView (beta version) - No major change in the Open edX theme fixes: LEARNER-9783 * feat: [FC-0047] Calendar main screen and dialogs (#322) * feat: Created calendar setting screen * feat: CalendarAccessDialog * feat: NewCalendarDialog * fix: Fixes according to PR feedback * fix: DiscussionTopicsViewModelTest.kt jUnit test * fix: assignment dates * feat: [FC-0047] Improved Dashboard Level Navigation (#308) * feat: Created Learn screen. Added course/program navigation. Added endpoint for UserCourses screen. * feat: Added primary course card * feat: Added start/resume course button * feat: Added alignment items * feat: Fix future assignment date, add courses list, add onSearch and onCourse clicks * feat: Add feature flag for enabling new/old dashboard screen, add UserCoursesScreen onClick methods * feat: Create AllEnrolledCoursesFragment. Add endpoint parameters * feat: AllEnrolledCoursesFragment UI * feat: Minor code refactoring, show cached data if no internet connection * feat: UserCourses screen data caching * feat: Dashboard * refactor: Dashboard type flag change, start course button change * feat: Added programs fragment to LearnFragment viewPager * feat: Empty states and settings button * fix: Number of courses * fix: Minor UI changes * fix: Fixes according to designer feedback * fix: Fixes after demo * refactor: Move CourseContainerTab * fix: Fixes according to PR feedback * fix: Fixes according to PR feedback * feat: added a patch from Omer Habib * fix: Fixes according to PR feedback * fix: Assignment date string * fix: Lint error * fix: Assignment date string * fix: Fixes according to PR feedback * fix: Fixes according to designer feedback * fix: Fixes according to PR feedback --------- Co-authored-by: Volodymyr Chekyrta <volodymyr.chekyrta@raccoongang.com> Co-authored-by: Farhan Arshad <43750646+farhan-arshad-dev@users.noreply.github.com>
https://www.figma.com/file/iZ56YMjbRMShCCDxqrqRrR/Open-edX-Mobile-App-All-Screens-v2.1?type=design&mode=design&t=W1kb1UTSIkmV2qox-0
The drop-down menu is enabled automatically when "Programs" is enabled in the configuration file.
Screen.Recording.2024-05-14.at.17.37.00.mov
Screen.Recording.2024-05-14.at.17.54.37.mov
Feature enabled by default.
🚨 You should use key:
to enable a list style Dashboard (old style)
API
Since the new APIs are not available in the master branch, please use the sandbox:
During the development process, we identified the need to refactor the ProgramsFragment.
Link to Issue: #325