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: Add support for Course Dates tab #80

Merged
merged 1 commit into from
Nov 8, 2023
Merged

feat: Add support for Course Dates tab #80

merged 1 commit into from
Nov 8, 2023

Conversation

farhan-arshad-dev
Copy link
Contributor

@farhan-arshad-dev farhan-arshad-dev commented Oct 30, 2023 •
edited
Loading

This PR adds support for Course Dates.

This is a preliminary PR with the Course Dates feature.
Colors are TBD for both Light and Dark themes.

Note:
Not included in this ticket:

  • Calendar integration.
  • PLS (shift due dates) banner.
  • Linking of items on the dates page to specific assessments within the app.

Sample Video

Screen_recording_20231102_153319.mp4

@farhan-arshad-dev farhan-arshad-dev changed the title Dates Tab Rewrite feat: Add support for Course Dates tab Oct 30, 2023
@volodymyr-chekyrta volodymyr-chekyrta requested review from k1rill and removed request for eyatsenkoperpetio October 30, 2023 13:13
@HamzaIsrar12 HamzaIsrar12 self-requested a review October 31, 2023 08:37
volodymyr-chekyrta
volodymyr-chekyrta requested changes Oct 31, 2023
View reviewed changes
build.gradle Outdated Show resolved Hide resolved
core/src/main/java/org/openedx/core/CourseDatesBadge.kt Outdated Show resolved Hide resolved
core/src/main/java/org/openedx/core/data/model/CourseDates.kt Outdated Show resolved Hide resolved
core/src/main/java/org/openedx/core/domain/model/CourseDateBlock.kt Outdated Show resolved Hide resolved
core/src/main/java/org/openedx/core/ui/theme/Color.kt Outdated Show resolved Hide resolved
core/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
course/src/main/res/drawable/ic_lock.xml Outdated Show resolved Hide resolved
course/src/main/java/org/openedx/course/presentation/dates/CourseDatesFragment.kt Outdated Show resolved Hide resolved
@volodymyr-chekyrta
Copy link
Contributor

@farhan-arshad-dev I added some comments while reviewing the code.
Anyway, thanks for the nice code, I enjoyed reviewing it.

@miankhalid
Copy link

@farhan-arshad-dev can you add some screenshots of the implementation to this PR? thanks!

k1rill
k1rill reviewed Nov 1, 2023
View reviewed changes
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.

@farhan-arshad-dev I added some cosmetical comments, some of them with Nit prefix are not important. In general nice code!

course/src/main/java/org/openedx/course/presentation/dates/DashboardUIState.kt Show resolved Hide resolved
course/src/main/java/org/openedx/course/presentation/container/CourseContainerFragment.kt Show resolved Hide resolved
core/src/main/java/org/openedx/core/domain/model/CourseDateBlock.kt Outdated Show resolved Hide resolved
core/src/main/java/org/openedx/core/data/model/CourseDates.kt Outdated Show resolved Hide resolved
course/src/main/java/org/openedx/course/presentation/dates/CourseDatesFragment.kt Show resolved Hide resolved
course/src/main/java/org/openedx/course/presentation/dates/CourseDatesFragment.kt Outdated Show resolved Hide resolved
course/src/main/java/org/openedx/course/presentation/dates/CourseDatesFragment.kt Outdated Show resolved Hide resolved
course/src/main/java/org/openedx/course/presentation/dates/CourseDatesFragment.kt Outdated Show resolved Hide resolved
course/src/main/java/org/openedx/course/presentation/dates/CourseDatesFragment.kt Outdated Show resolved Hide resolved
HamzaIsrar12
HamzaIsrar12 requested changes Nov 2, 2023
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.

Few things I noticed from the manual review:

  • The lock icon used in the Verified Only badge doesn't look center-aligned.



  • The same date has two different points.

course/src/main/java/org/openedx/course/presentation/dates/CourseDatesFragment.kt Outdated Show resolved Hide resolved
course/src/main/java/org/openedx/course/presentation/dates/CourseDatesFragment.kt Show resolved Hide resolved
core/src/main/java/org/openedx/core/data/api/CourseApi.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 Show resolved Hide resolved
core/src/main/java/org/openedx/core/data/model/CourseDates.kt Outdated Show resolved Hide resolved
core/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
course/src/main/java/org/openedx/course/presentation/container/CourseContainerFragment.kt Show resolved Hide resolved
course/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
course/src/main/res/menu/bottom_course_container_menu.xml Outdated Show resolved Hide resolved
@HamzaIsrar12
Copy link
Contributor

@volodymyr-chekyrta, when looking into the docs, it's considered good practice to pass the Modifier to every Composable. Should we start implementing this approach? 🤔

cc: @farhan-arshad-dev @k1rill

@volodymyr-chekyrta
Copy link
Contributor

@HamzaIsrar12 sounds good!
Not sure about every Composable, we have *Screen functions that are only used once, but we can take that as a rule for component/reusable functions.

volodymyr-chekyrta
volodymyr-chekyrta previously approved these changes Nov 6, 2023
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.

LGTM

k1rill
k1rill previously approved these changes Nov 6, 2023
View reviewed changes
@volodymyr-chekyrta
Copy link
Contributor

@farhan-arshad-dev, it appears everything is set for the merge.
Could you please resolve conflicts so we can proceed with the merge?

HamzaIsrar12
HamzaIsrar12 approved these changes Nov 8, 2023
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.

LGTM 🏎️

@farhan-arshad-dev
feat: add Course Dates Tab
5e6666f 
- Add Course dates support.
- Add test cases for Course dates ViewModel.

Not included in this PR:
- Calendar integration
- PLS (shift due dates) banner
- Linking of items on the dates page to specific assessments within the app.

fixes: LEARNER-9664
@farhan-arshad-dev
Copy link
Contributor Author

@volodymyr-chekyrta This PR is ready to merge.

@volodymyr-chekyrta volodymyr-chekyrta merged commit b23156f into openedx:develop Nov 8, 2023
2 checks passed
@k1rill k1rill linked an issue Dec 5, 2023 that may be closed by this pull request
[Android] "Dates" tab for Courses #104
Closed
@farhan-arshad-dev farhan-arshad-dev deleted the farhan_ar/LEARNER-9664 branch June 24, 2024 06:01
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

@k1rill k1rill k1rill left review comments

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

Successfully merging this pull request may close these issues.

[Android] "Dates" tab for Courses
5 participants
@farhan-arshad-dev @volodymyr-chekyrta @miankhalid @HamzaIsrar12 @k1rill