Skip to content

feat: [FC-86] Added filtration for Catalog page#33

Merged
brian-smith-tcril merged 2 commits intoopenedx:masterfrom
raccoongang:Peter_Kulko/catalog-page-data-table-filtration
Oct 21, 2025
Merged

feat: [FC-86] Added filtration for Catalog page#33
brian-smith-tcril merged 2 commits intoopenedx:masterfrom
raccoongang:Peter_Kulko/catalog-page-data-table-filtration

Conversation

@PKulkoRaccoonGang
Copy link
Contributor

@PKulkoRaccoonGang PKulkoRaccoonGang commented Oct 9, 2025

Note

Search functionality will be added in the next PR.

Description

This PR adds course filtering for the Course Catalog page.

Useful information

Initial setup

  1. Clone and install the tutor-mfe plugin from the draft PR that adds support for Catalog MFE.
  2. Go to http://apps.local.openedx.io:1998/catalog/courses
  3. Change the DEFAULT_PAGE_SIZE value to 2, or create more than 20 courses

How Has This Been Tested?

  1. Go to Course Catalog page (http://apps.local.openedx.io:1998/catalog/courses).
  2. Facets and facet options are fetched from the course_list_search API and displayed correctly.
  3. Full facet option names are displayed (e.g., the "en" language code is shown as "English").
  4. Each table interaction (filtering, pagination, etc.) triggers a separate backend request with the corresponding parameters.
  5. Skeleton cards are displayed while data is loading during pagination.
  6. The user can navigate between pagination pages using either the dropdown or the arrow buttons (default DataTable component functionality).
  7. The sidebar displays the number of courses next to each facet option.
  8. The user can filter results by selecting multiple facet options:
image 10. Filtering can be applied across multiple facets simultaneously: image 11. The user can clear all selected filters by clicking the Clear filters button (default DataTable component functionality). 12. Based on the selected facet options within one facet, the available options in other facets dynamically update.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 9, 2025
@openedx-webhooks
Copy link

openedx-webhooks commented Oct 9, 2025

Thanks for the pull request, @PKulkoRaccoonGang!

This repository is currently maintained by @openedx/committers-frontend.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.
🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads
🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details
Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@github-project-automation github-project-automation bot moved this to Needs Triage in Contributions Oct 9, 2025
@PKulkoRaccoonGang PKulkoRaccoonGang changed the title feat: Added filtration for Catalog page feat: [FC-86] Added filtration for Catalog page Oct 9, 2025
@codecov
Copy link

codecov bot commented Oct 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.45%. Comparing base (9c3bf5c) to head (30f0503).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
+ Coverage   98.15%   98.45%   +0.29%     
==========================================
  Files          73       75       +2     
  Lines         597      712     +115     
  Branches       82      114      +32     
==========================================
+ Hits          586      701     +115     
  Misses         11       11              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines 6 to 8
import {
CourseListSearchResponse, CourseListSearchParams, CourseListSearchHook, DataTableParams,
} from './types';

Choose a reason for hiding this comment

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

[question] Should we add type to declare the import type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, added

@PKulkoRaccoonGang PKulkoRaccoonGang force-pushed the Peter_Kulko/catalog-page-data-table-filtration branch from 7c60241 to cae1faf Compare October 15, 2025 07:49
@mphilbrick211 mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Oct 15, 2025
@mphilbrick211 mphilbrick211 moved this from Needs Triage to Ready for Review in Contributions Oct 15, 2025
fix: corrected displayName for filters

test: added tests

refactor: tests refactoring

refactor: after review
Copy link
Contributor

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

I started a code review and left a couple minor comments, then I pulled the branch to test locally and I noticed a couple of issues.

Edit: The commit I tested for this review was cae1faf

Screencast.From.2025-10-17.09-20-06.mp4
  • The checkboxes aren't getting checked
  • The page is flashing a lot when filters are changed. I think one of the main reasons it is so noticeable is that the footer is moving. I think making sure the footer doesn't bounce up and down when the number of rows of result cards changes is definitely something we should address for this page.

@brian-smith-tcril
Copy link
Contributor

Did another screen capture to break down why it feels like there's so much flashing when changing filters

Screencast.From.2025-10-17.09-29-43.mp4

Before clicking the filter

Screenshot From 2025-10-17 09-30-35

Clicking - goes to loading state

Screenshot From 2025-10-17 09-30-47

Loaded, card cap image hasn't loaded

Screenshot From 2025-10-17 09-30-59

Fully loaded

Screenshot From 2025-10-17 09-31-06

This makes me think that it's more than just the footer bouncing that's an issue here. That being said, the footer bouncing is made worse by the fact that it moves multiple times (first it's below the 2 rows of results, then it's below the 1 loading state row, then it's below the one row that doesn't have an image cap, then it's below the single loaded row).

@brian-smith-tcril
Copy link
Contributor

After checking the Paragon docs site page for Card it seems the image cap issue is definitely in Paragon itself.

We should probably make a Paragon issue for better handling loading states in CardImageCap.

@PKulkoRaccoonGang PKulkoRaccoonGang force-pushed the Peter_Kulko/catalog-page-data-table-filtration branch from cae1faf to 632d372 Compare October 20, 2025 05:55
@PKulkoRaccoonGang PKulkoRaccoonGang force-pushed the Peter_Kulko/catalog-page-data-table-filtration branch from 632d372 to 30f0503 Compare October 20, 2025 06:00
@PKulkoRaccoonGang
Copy link
Contributor Author

PKulkoRaccoonGang commented Oct 20, 2025

@brian-smith-tcril thanks!

The checkboxes aren't getting checked

I performed the following steps and didn’t encounter any issues with the checkboxes:

rm -rf node_modules
nvm use
npm ci
npm run dev

After running npm list, I can see that I have @openedx/[email protected] installed (the latest version at the moment).
I'm looking for other options that could cause this problem.

After checking the Paragon docs site page for Card it seems the image cap issue is definitely in Paragon itself.

We should probably make a Paragon issue for better handling loading states in CardImageCap.

We already have an existing issue: openedx/paragon#2480 for this problem, as well as a PR: openedx/paragon#2846 that attempted to fix it. However, as far as I remember, it didn’t completely resolve the issue.

This makes me think that it's more than just the footer bouncing that's an issue here. That being said, the footer bouncing is made worse by the fact that it moves multiple times (first it's below the 2 rows of results, then it's below the 1 loading state row, then it's below the one row that doesn't have an image cap, then it's below the single loaded row).

I think the situation will improve a bit once we have the footer fixed to the bottom and after the updates to the Paragon Date view component.

It might also make sense to adjust the values of skeletonCardCount and DEFAULT_PAGE_SIZE to display more cards - both for the actual course list and the skeleton loaders. This way, we can improve the layout for cases where the platform has many courses, and the Date view will typically show two or three rows of cards (depending on the specific values set for skeletonCardCount and DEFAULT_PAGE_SIZE).

@PKulkoRaccoonGang
Copy link
Contributor Author

UPD: @Serj-N also tried running the project locally on this branch, and he didn’t encounter any issues with the checkboxes either

image

@brian-smith-tcril
Copy link
Contributor

brian-smith-tcril commented Oct 21, 2025

@PKulkoRaccoonGang I pulled the latest version of this branch (30f0503) and the checkboxes are working as expected now! I'm thinking when I ran into the problem before it might have been a browser cache issue.

@brian-smith-tcril
Copy link
Contributor

We already have an existing issue: openedx/paragon#2480 for this problem, as well as a PR: openedx/paragon#2846 that attempted to fix it. However, as far as I remember, it didn’t completely resolve the issue.

Thanks for digging that up!

I think the situation will improve a bit once we have the footer fixed to the bottom

I 100% agree, fixing the footer to the bottom will definitely be a good short-term fix that will help quite a bit.

and after the updates to the Paragon DataView component.

Definitely! Getting Paragon to a point where the image cap doesn't disappear/reappear when images are loading will be a huge UX win.

It might also make sense to adjust the values of skeletonCardCount and DEFAULT_PAGE_SIZE to display more cards - both for the actual course list and the skeleton loaders. This way, we can improve the layout for cases where the platform has many courses, and the Date view will typically show two or three rows of cards (depending on the specific values set for skeletonCardCount and DEFAULT_PAGE_SIZE).

That definitely seems like something worth looking into at some point. Might be good to make an issue on this repo to investigate behavior with different values to find good defaults, but I think that can happen after this FC is closed out.

Copy link
Contributor

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

LGTM!

All of the UX issues I noticed in my reviews are documented as issues on the Paragon repo, so this is good to land!

@brian-smith-tcril brian-smith-tcril merged commit a71048e into openedx:master Oct 21, 2025
6 checks passed
@github-project-automation github-project-automation bot moved this from Ready for Review to Done in Contributions Oct 21, 2025
@PKulkoRaccoonGang
Copy link
Contributor Author

That definitely seems like something worth looking into at some point. Might be good to make an issue on this repo to investigate behavior with different values to find good defaults, but I think that can happen after this FC is closed out.

Created a separate issue to track this problem: #41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

FC Relates to an Axim Funded Contribution project 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.

5 participants