-
Notifications
You must be signed in to change notification settings - Fork 123
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
feat: pagination studio home for courses #825
Conversation
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:
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. |
bf36663
to
69fec99
Compare
Codecov ReportAttention: Patch coverage is
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. |
69fec99
to
75832f8
Compare
75832f8
to
94be906
Compare
95835b5
to
d6e2e91
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.
All my comments were addressed, so this is looking good to me!
3d3d359
to
1f6d30f
Compare
@openedx/2u-tnl this is ready for review! |
Thanks for testing this, it was the number of courses that was being shown I added a fix for that |
Hi @arbrandes! Could take a look over here please? Thanks a lot |
In the backend, we're using a feature toggle to turn on/off the API: |
That said I added the feature with the same name as it is in backend |
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. |
Hi @jesperhodge we aren't planning to implement pagination for the libraries tab, our scope is only for courses tab |
The first time the pagination loads, it shows: Screencast.from.22-03-24.12.40.12.mp4 |
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.
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!
I won't be able to review this in a week, so I will check for any changes on April 1st |
Hello there, @jesperhodge @arbrandes! Can you help us with a review? Let us know! |
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.
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.
dae083d
to
1bdf719
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.
Thanks a lot, John! Again, sorry for the late review. This is great work, super happy to have pagination now!!
@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. |
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 listenable this feature flag
new_studio_mfe.use_new_home_page
DEMO
demo_course_home_pagination.mp4
Course cards
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
Set up your edx-platform
Check it out this branch inside frontend-app-course-authoring
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