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

Course navigation functionality #167

Merged
merged 18 commits into from
Dec 26, 2023

Conversation

dixidroid
Copy link
Contributor

@dixidroid dixidroid commented Dec 14, 2023

  • Expandable items for the Course and Video tabs on the course screen.

COURSE_NESTED_LIST_ENABLED feature flag has been added to enable this functionality (false by default)

Screen_Recording_20231214_153752_OpenEdX.mp4
  • Hide the top banner on the Course and Video tabs.

COURSE_BANNER_ENABLED feature flag has been added to hide the top banner (visible by default)

  • The bottom tab bar has been moved to top within the course screen.

COURSE_TOP_TAB_BAR_ENABLED feature flag has been added to enable this functionality (false by default)

Screen_Recording_20231214_153907_OpenEdX.mp4
  • Horizontal xBlock's indicator on the opened unit screen. Replaced the Vertical indicator with a horizontal one.

COURSE_UNIT_PROGRESS_ENABLED feature flag has been added to turn this functionality (false by default)

Screen_Recording_20231214_153946_OpenEdX.mp4

@HamzaIsrar12
Copy link
Contributor

HamzaIsrar12 commented Dec 18, 2023

Hey @dixidroid, I completed the initial manual review last week and everything was working fine but just noticed these two issues:

  • The subsection screen becomes visible when navigating with the Resume button or from the course dates hyperlinks.
  • The app is crashing while navigating because of NetworkOnMainThreadException in the background. I'm not sure why yet.

And I will share the Code Review points by day end today. 👍🏻

@dixidroid
Copy link
Contributor Author

dixidroid commented Dec 18, 2023

Hi @HamzaIsrar12
About the crash - it's not related to this PR. It happens elsewhere. @nizmaylova already found this crash and created a new issue - #152
I'm going to fix it this week.

About the Resume button - we have a ticket for it #127 but it's already assigned to @moiz994
This is why this functionality is not included.

Thanks!

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.

Hi @dixidroid, I have posted the review for the Section and SubSection layer. Hopefully, I will post the review for the Unit and Component level tomorrow.

Great work overall. ✨

@dixidroid dixidroid force-pushed the feature/course_navigation branch from f3bd988 to 9b9116b Compare December 19, 2023 15:46
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.

Some minor code changes have been made at both the unit and component levels.

@dixidroid dixidroid force-pushed the feature/course_navigation branch from ebdd126 to a0fc823 Compare December 22, 2023 10:04
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.

One minor nit due to multiple feature flags. 💥

With isCourseNestedListEnabled set to false, moving to a component shows the title of the unit. In the develop branch, no title is displayed for the component.

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. Excellent job! 🏎️ ✨

@volodymyr-chekyrta volodymyr-chekyrta merged commit c390c82 into openedx:develop Dec 26, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants