Skip to content

feat: pagination studio home for courses #825

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

Merged
merged 17 commits into from
Apr 3, 2024

Conversation

johnvente
Copy link
Contributor

@johnvente johnvente commented Feb 6, 2024

Description

This PR adds pagination for the studio home view and makes minor changes to each course card.

NOTE: This needs to be activated by the environment variable ENABLE_HOME_PAGE_COURSE_API_V2 otherwise, it will continue using the old course list

enable this feature flag
new_studio_mfe.use_new_home_page

DEMO

demo_course_home_pagination.mp4
Screen Shot 2024-03-22 at 1 08 51 PM

Course cards

BEFORE NOW

Supporting information

To review a little bit more about this implementation, please check this:
https://openedx.atlassian.net/wiki/spaces/OEPM/pages/4032856101/Studio+Home+incremental+upgrades+-+product+approach

How to test it

  1. Set up your edx-platform

     cd path-to-your-edx-platform
     git fetch origin pull/34175/head:MJG/course-home-api-v2-pagination
     git checkout MJG/course-home-api-v2-pagination
    
  2. Check it out this branch inside frontend-app-course-authoring

     cd path-to-frontend-app-course-authoring
     git fetch origin pull/825/head:jv/feat-home-studio-pagination
     git checkout jv/feat-home-studio-pagination
     npm start
    
  3. The default items per page is 10 so you would need to create more than 10 courses

You can create this tutor plugin to enable it

name: mfe_course_authoring_config
version: 0.1.0
patches:
  mfe-lms-development-settings: |
    MFE_CONFIG["ENABLE_HOME_PAGE_COURSE_API_V2"]=True

  mfe-lms-production-settings: |
    MFE_CONFIG["ENABLE_HOME_PAGE_COURSE_API_V2"]=True

@openedx-webhooks
Copy link

openedx-webhooks commented Feb 6, 2024

Thanks for the pull request, @johnvente! 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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 6, 2024
@johnvente johnvente force-pushed the jv/feat-home-studio-pagination branch from bf36663 to 69fec99 Compare February 6, 2024 22:00
Copy link

codecov bot commented Feb 6, 2024

Codecov Report

Attention: Patch coverage is 92.72727% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 91.93%. Comparing base (784a811) to head (1bdf719).
Report is 8 commits behind head on master.

Files Patch % Lines
src/studio-home/hooks.jsx 75.00% 2 Missing ⚠️
src/studio-home/tabs-section/courses-tab/index.jsx 85.71% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #825      +/-   ##
==========================================
+ Coverage   91.87%   91.93%   +0.06%     
==========================================
  Files         574      571       -3     
  Lines       10078    10230     +152     
  Branches     2157     2228      +71     
==========================================
+ Hits         9259     9405     +146     
- Misses        791      798       +7     
+ Partials       28       27       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@johnvente johnvente marked this pull request as ready for review February 6, 2024 22:09
@johnvente johnvente force-pushed the jv/feat-home-studio-pagination branch from 69fec99 to 75832f8 Compare February 7, 2024 00:58
@johnvente johnvente force-pushed the jv/feat-home-studio-pagination branch from 75832f8 to 94be906 Compare February 7, 2024 01:12
@mphilbrick211 mphilbrick211 requested a review from a team February 7, 2024 15:09
@johnvente johnvente force-pushed the jv/feat-home-studio-pagination branch from 95835b5 to d6e2e91 Compare February 8, 2024 18:15
Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

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

All my comments were addressed, so this is looking good to me!

@johnvente johnvente force-pushed the jv/feat-home-studio-pagination branch from 3d3d359 to 1f6d30f Compare February 8, 2024 19:44
@mphilbrick211
Copy link

@openedx/2u-tnl this is ready for review!

@mphilbrick211 mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Feb 21, 2024
@johnvente johnvente requested a review from a team as a code owner March 19, 2024 15:34
@mariajgrimaldi
Copy link
Member

I was testing this out in one of my environments and found that when a user doesn't have course creator permissions, this is what is shown:
image
This also happens when no courses are listed for the current user. I'm guessing that's the number of pages. Should we remove it?

@johnvente
Copy link
Contributor Author

johnvente commented Mar 19, 2024

I was testing this out in one of my environments and found that when a user doesn't have course creator permissions, this is what is shown:This also happens when no courses are listed for the current user. I'm guessing that's the number of pages. Should we remove it?

Thanks for testing this, it was the number of courses that was being shown I added a fix for that

@johnvente
Copy link
Contributor Author

Hi @arbrandes! Could take a look over here please? Thanks a lot

@mariajgrimaldi
Copy link
Member

In the backend, we're using a feature toggle to turn on/off the API: FEATURES['ENABLE_HOME_PAGE_COURSE_API_V2']. Can we use something similar here and in #846?

@johnvente
Copy link
Contributor Author

In the backend, we're using a feature toggle to turn on/off the API: FEATURES['ENABLE_HOME_PAGE_COURSE_API_V2']. Can we use something similar here and in #846?

That said I added the feature with the same name as it is in backend ENABLE_HOME_PAGE_COURSE_API_V2

@jesperhodge
Copy link
Member

Hi @johnvente , I'm reviewing this for TNL. In addition, I'm also looking at loading times of studio home -> libraries tab. I wanted to ask if you or edunext are planning to implement pagination for the libraries page as well? I want to avoid conflicts or duplications there.

@johnvente
Copy link
Contributor Author

Hi @jesperhodge we aren't planning to implement pagination for the libraries tab, our scope is only for courses tab

@mariajgrimaldi mariajgrimaldi self-requested a review March 22, 2024 15:24
@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Mar 22, 2024

The first time the pagination loads, it shows: Showing 44 of 44. Here's a video:

Screencast.from.22-03-24.12.40.12.mp4

Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

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

I tested this in my local environment, and the pagination is working as expected. I turned it on/off, and tested that everything worked as before. Thank you for addressing all my comments!

@johnvente
Copy link
Contributor Author

johnvente commented Mar 22, 2024

I won't be able to review this in a week, so I will check for any changes on April 1st

@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Apr 1, 2024

Hello there, @jesperhodge @arbrandes! Can you help us with a review? Let us know!

Copy link
Member

@jesperhodge jesperhodge left a comment

Choose a reason for hiding this comment

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

Looks very good to me! I just left some very minor comments but everything else looks great.
Apologies for not responding here for a week, I was out sick.

@johnvente johnvente force-pushed the jv/feat-home-studio-pagination branch from dae083d to 1bdf719 Compare April 2, 2024 20:23
Copy link
Member

@jesperhodge jesperhodge left a comment

Choose a reason for hiding this comment

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

Thanks a lot, John! Again, sorry for the late review. This is great work, super happy to have pagination now!!

@jesperhodge jesperhodge merged commit fde3872 into openedx:master Apr 3, 2024
@openedx-webhooks
Copy link

@johnvente 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

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
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants