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

feat: [FC-0047] Improved Dashboard Level Navigation #308

Merged
merged 29 commits into from
May 30, 2024

Conversation

PavloNetrebchuk
Copy link
Contributor

@PavloNetrebchuk PavloNetrebchuk commented May 2, 2024

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

Light Theme Dark Theme
Screenshot 2024-05-14 at 17 53 14 Screenshot 2024-05-14 at 17 53 36
Screenshot 2024-05-10 at 17 21 21 Screenshot 2024-05-10 at 17 21 38
Screenshot 2024-05-10 at 17 21 59 Screenshot 2024-05-10 at 17 22 15
Screenshot 2024-05-10 at 17 27 50 Screenshot 2024-05-10 at 17 28 08

Feature enabled by default.

🚨 You should use key:

DASHBOARD:
       TYPE: "list"

to enable a list style Dashboard (old style)

API

Since the new APIs are not available in the master branch, please use the sandbox:

API_HOST_URL: 'https://axim-mobile.raccoongang.net'
OAUTH_CLIENT_ID: 'zP3vPz00c8fTRpYjNbVSlA1fxt9LnCxTM4JK1KQ0'

During the development process, we identified the need to refactor the ProgramsFragment.
Link to Issue: #325

# 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
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 2, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented May 2, 2024

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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@volodymyr-chekyrta volodymyr-chekyrta changed the title Feat: Dashboard feat: [FC-0047] Improved Dashboard Level Navigation May 9, 2024
@volodymyr-chekyrta volodymyr-chekyrta added the product review PR requires product review before merging label May 10, 2024
@volodymyr-chekyrta volodymyr-chekyrta marked this pull request as ready for review May 10, 2024 14:47
@sdaitzman
Copy link

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:

  1. In light and dark mode, the Resume course quick action doesn’t need a border separating it from the other actions. Generally, the quick actions only need a separating border when they’re the same color and would otherwise be difficult to distinguish.
  2. In dark mode, I think the View All Courses card may be a bit too dark/difficult to distinguish from the background. It should use the half dark color: Figma Link
  3. In the All Courses view in light mode, the filter options currently use a light gray border color, and keep that border color when selected. They should use the accent color as their border, and have no border when selected.
  4. Similarly, in dark mode, course filter options should have no border and should just change their background from Half Dark to Accent when selected.
  5. When the Programs/Courses dropdown switcher is tapped, the dropdown icon should animate if possible (either fade or morph from one version to the other)

Android:

  1. In the primary course card, the title should cut off with an ellipse at three lines
  2. Some of the other iOS minor visual issues identified above may also apply, but I couldn’t see them in the video to be sure (e.g. the View All Courses card in dark mode)
  3. When the Programs/Courses dropdown switcher is tapped, the dropdown icon should switch to its upside-down version, and animate if possible (either fade or morph from one version to the other)

Both iOS and Android:

  1. Copy edits: “2 Past Due Assignment” should be “2 Past Due Assignments.”
  2. The account settings gear in the top right should switch to the updated icon (this change is intended to make it clear on all dashboard-level tabs that the account settings icon is not a settings control for the specific feature on that tab, based on input from the Open edX Mobile Design Weekly group): Figma link
  3. Selected dashboard-level tabs in dark mode should switch over to using the Dark Theme Accent Text color (#879FF5), not Accent, for adequate text/icon contrast (this could fall under a future cleanup)
  4. Currently, the deep-link from some quick actions like upcoming assignments/resume course jump to the course home, pause a brief moment, then navigate into the content. If possible, it would be better to navigate directly to the content loading screen.

@PavloNetrebchuk
Copy link
Contributor Author

Hello, @sdaitzman
I made the changes based on your feedback and changed screenshots and videos in the PR description.

# 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
Copy link
Contributor

@k1rill k1rill left a 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

Copy link
Contributor

@omerhabib26 omerhabib26 left a 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!"

@@ -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>
Copy link
Contributor

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>
Copy link
Contributor

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.

Copy link
Contributor

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") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Requires auto-format.

null
}
}

Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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(),
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

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(
Copy link
Contributor

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?

Copy link
Contributor

@k1rill k1rill left a 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

# Conflicts:
#	app/src/main/res/layout/fragment_main.xml
#	course/src/main/java/org/openedx/course/presentation/ui/CourseUI.kt
Copy link
Contributor

@omerhabib26 omerhabib26 left a 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) {
Copy link
Contributor

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()
        }
    }
}

WEBVIEW:
PROGRAM_URL: ''
PROGRAM_DETAIL_URL_TEMPLATE: ''

DASHBOARD:
TYPE: 'primary_course'
Copy link
Contributor

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

app/src/main/res/layout/fragment_main.xml Show resolved Hide resolved

fun getExternalAppDir(context: Context): File {
fun getExternalAppDir(): File {
Copy link
Contributor

Choose a reason for hiding this comment

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

}

override fun getProgramFragmentInstance(): Fragment {
return ProgramFragment(myPrograms = true, isNestedFragment = true)
Copy link
Contributor

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(),
Copy link
Contributor

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 {
Copy link
Contributor

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 "",
Copy link
Contributor

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 {
Copy link
Contributor

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(),
Copy link
Contributor

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.

k1rill
k1rill previously approved these changes May 29, 2024
Copy link
Contributor

@k1rill k1rill left a comment

Choose a reason for hiding this comment

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

LGTM!

omerhabib26
omerhabib26 previously approved these changes May 29, 2024
# Conflicts:
#	dashboard/src/main/java/org/openedx/courses/presentation/AllEnrolledCoursesFragment.kt
#	dashboard/src/main/java/org/openedx/courses/presentation/DashboardGalleryViewModel.kt
@PavloNetrebchuk PavloNetrebchuk dismissed stale reviews from omerhabib26 and k1rill via 2f02b39 May 29, 2024 14:07
@PavloNetrebchuk
Copy link
Contributor Author

During the development process, we identified the need to refactor the ProgramsFragment.
Link to Issue: #325

# 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
@volodymyr-chekyrta volodymyr-chekyrta merged commit c2684f9 into openedx:develop May 30, 2024
3 checks passed
@openedx-webhooks
Copy link

@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.

@volodymyr-chekyrta volodymyr-chekyrta deleted the feat/dashboard branch May 30, 2024 08:11
PavloNetrebchuk added a commit to raccoongang/openedx-app-android that referenced this pull request May 30, 2024
* 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
volodymyr-chekyrta added a commit that referenced this pull request Jun 6, 2024
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U product review PR requires product review before merging
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants