fix: library dropdowns behavior #2210

DanielVZ96 · 2025-06-22T17:45:52Z

Description

Changes:

Refactored SearchFilterWidget to use Dropdown component instead of ModalPopup in order to be able to close the current filter and interact with other elements in a single click.
Fixed dropdown coordination in BaseCard component to allow proper event propagation
Minor spacing adjustment in filter by published

Supporting information

Testing instructions

On a library v2:

Open any of the following filters: type, publish status, tags
Open another filter
Assert only a single click is needed to close and open a new filter
Create multiple library units, sections/subsections, collections, components
Open the dropdown menu of each one of them as shown in the picture

Assert that each time you open a new dropdown, the previous open one is closed.

Updated the click event prevention in the BaseCard component to allow dropdown toggle clicks to propagate, ensuring proper coordination between dropdowns.

Replaced the ModalPopup with a Dropdown for better user experience. The filtering dropdowns (publish status/ component type/ tags) all close out if a user clicks anywhere outside of the dropdown menus. In this implementation, however, clicking outside of the opened menu only triggers closing the menu. As in: if a filtering dropdown is open, and decide you want to open a different filtering option, a user has to click twice: once to close the initially opened dropdown, and a second time to open the second filtering option.

openedx-webhooks · 2025-06-22T17:45:57Z

Thanks for the pull request, @DanielVZ96!

This repository is currently maintained by @bradenmacdonald.

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.

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.

codecov · 2025-06-24T11:18:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.17%. Comparing base (aeefcc6) to head (2314cb8).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2210      +/-   ##
==========================================
+ Coverage   94.15%   94.17%   +0.01%     
==========================================
  Files        1164     1164              
  Lines       24599    24604       +5     
  Branches     5381     5367      -14     
==========================================
+ Hits        23162    23170       +8     
- Misses       1360     1366       +6     
+ Partials       77       68       -9

☔ 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.

pomegranited

Hi @DanielVZ96 , this is working great for the main Library page!
We just need the same behavior on the Unit and Section/Subsection pages too.

pomegranited · 2025-06-24T17:46:19Z

src/library-authoring/components/BaseCard.tsx

+                if (!isDropdownToggle) {
+                  e.stopPropagation();
+                }
+              }}


@DanielVZ96 Could you apply the same to the section/subsection child preview cards:

And to the Unit child preview cards:

pomegranited · 2025-06-24T17:48:53Z

src/search-manager/FilterByPublished.tsx

+        {filterLabels[filter]}{' '}
        <Badge variant="light" pill>{publishStatus[filter] ?? 0}</Badge>


Actually, our translated filter labels should be formatted text that includes a variable for this count badge, so that RTL languages can opt to put the count on the left side of the text label. But since that's out of scope for this change it's ok to do as a follow-up PR.

CC @ChrisChV

Alternatively, this spacing can be handled with CSS.

@DanielVZ96 I agree with Jill, could you use CSS for the spacing ({' '})?

Actually, our translated filter labels should be formatted text that includes a variable for this count badge, so that RTL languages can opt to put the count on the left side of the text label. But since that's out of scope for this change it's ok to do as a follow-up PR.

I will create an issue for this

src/search-manager/SearchFilterWidget.tsx

pomegranited

Thank you for those fixes @DanielVZ96 ! I left a couple of nits, but this is working perfectly now.

👍

I tested this using the PR test instructions
I read through the code
I checked for accessibility issues by using my keyboard to navigate
~~Includes documentation~~ N/A
User-facing strings are extracted for translation

pomegranited · 2025-06-26T13:01:44Z

src/search-manager/FilterByPublished.tsx

+        {filterLabels[filter]}{' '}
        <Badge variant="light" pill>{publishStatus[filter] ?? 0}</Badge>


Alternatively, this spacing can be handled with CSS.

pomegranited · 2025-06-26T13:03:57Z

src/library-authoring/section-subsections/LibraryContainerChildren.tsx

+        onClick={(e) => {
+          // Wrap the actions in a div to prevent the card from being clicked when the actions are clicked.
+          const target = e.target as HTMLElement;
+          const isDropdownToggle = target.closest('[data-testid="dropdown"], [data-testid="container-card-menu-toggle"], .pgn__dropdown-toggle-iconbutton');


I'm not comfortable with testid attributes being used in running code -- and they're even a last resort when identifying elements in tests. Can we use classes here instead?

And is there any way to avoid duplicating this logic every time we need it? E.g using a utility function, or at least only declaring the function once per file?

oh you are absolutely right! i refactored it into a function in each file. i couldn't find any place to put it as a shared utility function tbh.

pomegranited

Much better with those changes, thank you @DanielVZ96 👍

ChrisChV

@DanielVZ96 Is it possible to fix this without opening the sidebar when clicking the card menu (without selecting the card)? If it is not possible, could you ask Product to see if that behavior is correct (somewhere I remember reading that it is not)?

ChrisChV · 2025-06-27T13:21:39Z

src/search-manager/FilterByPublished.tsx

+        {filterLabels[filter]}{' '}
        <Badge variant="light" pill>{publishStatus[filter] ?? 0}</Badge>


@DanielVZ96 I agree with Jill, could you use CSS for the spacing ({' '})?

Actually, our translated filter labels should be formatted text that includes a variable for this count badge, so that RTL languages can opt to put the count on the left side of the text label. But since that's out of scope for this change it's ok to do as a follow-up PR.

I will create an issue for this

DanielVZ96 · 2025-06-27T18:42:04Z

@DanielVZ96 Is it possible to fix this without opening the sidebar when clicking the card menu (without selecting the card)? If it is not possible, could you ask Product to see if that behavior is correct (somewhere I remember reading that it is not)?

@edschema should we prevent the sidebar change/opening? here's a video that shows the sidebar opening when a card menu toggle is clicked.

Grabacion.de.pantalla.2025-06-27.a.la.s.8.37.58.p.m.mov

edschema · 2025-06-27T19:05:44Z

@DanielVZ96 Is it possible to fix this without opening the sidebar when clicking the card menu (without selecting the card)? If it is not possible, could you ask Product to see if that behavior is correct (somewhere I remember reading that it is not)?

@edschema should we prevent the sidebar change/opening? here's a video that shows the sidebar opening when a card menu toggle is clicked.

Grabacion.de.pantalla.2025-06-27.a.la.s.8.37.58.p.m.mov

Hi @DanielVZ96! This is our mistake: we should have created a ticket for this behavior. I apologize.

Clicking overflow menu should not select the card, and correspondingly should not open the sidebar for that object. If the user clicks either "Edit" or "add to collection" the sidebar should open. "Edit" opens the sidebar behind the edit modal. "Add to collection" should open the "manage" tab on the sidebar in the add to collection mode.

For container blocks, there is "open" so sidebar behavior does not apply since opening navigates to the container's page.

In unit views, the same pattern should persist. "edit" opens the sidebar behind the edit modal for component, "add to collection" opens the manage sidebar in edit mode.

For other container pages, Section/Subsection, should follow similar rules. "Open" navigates to a new page, so not relevant, "add to collection" shifts to a new collection.

DanielVZ96 added 3 commits June 22, 2025 14:32

fix: close base card dropdowns when opening another

70c3b57

Updated the click event prevention in the BaseCard component to allow dropdown toggle clicks to propagate, ensuring proper coordination between dropdowns.

fix: minor spacing adjustment in filter by published

61ecc58

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 22, 2025

openedx-webhooks added this to Contributions Jun 22, 2025

github-project-automation bot moved this to Needs Triage in Contributions Jun 22, 2025

chore: make ci run

5bf5613

DanielVZ96 marked this pull request as draft June 24, 2025 10:09

DanielVZ96 added 3 commits June 24, 2025 12:14

fix: filter test

92d9629

fix: keep clear filter code

fde7d3d

fix: search ui test

e48d1ca

DanielVZ96 marked this pull request as ready for review June 24, 2025 11:06

fix: lint

85f03a2

DanielVZ96 added 4 commits June 24, 2025 13:54

fix: improve coverage

c651e1f

fix: remove extra code not needed

661f46a

fix: improve coverage

4a1593d

fix: linter

b290b4e

DanielVZ96 requested a review from pomegranited June 24, 2025 16:53

pomegranited suggested changes Jun 24, 2025

View reviewed changes

DanielVZ96 added 3 commits June 25, 2025 11:43

fix: also apply to units blocks and section/subsection children

abd97f4

refactor: remove callback calling single clearfilter function

6dbd7a8

fix: improve coverage

a15e15b

DanielVZ96 requested a review from pomegranited June 26, 2025 10:38

pomegranited approved these changes Jun 26, 2025

View reviewed changes

DanielVZ96 added 3 commits June 26, 2025 09:15

Merge branch 'master' into dvz/single-overflow-visitble

6335c60

refactor: reduce code duplication and do not rely con testid

960961b

fix: remove conflict marker

2314cb8

DanielVZ96 requested a review from ChrisChV June 26, 2025 14:13

pomegranited approved these changes Jun 26, 2025

View reviewed changes

ChrisChV requested changes Jun 27, 2025

View reviewed changes

ChrisChV mentioned this pull request Jun 27, 2025

Updates FilterByPublished count labels to support intl #2229

Open

		{filterLabels[filter]}{' '}
		<Badge variant="light" pill>{publishStatus[filter] ?? 0}</Badge>

fix: library dropdowns behavior #2210

Are you sure you want to change the base?

fix: library dropdowns behavior #2210

Uh oh!

Conversation

DanielVZ96 commented Jun 22, 2025

Description

Supporting information

Testing instructions

Uh oh!

openedx-webhooks commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

ChrisChV left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielVZ96 commented Jun 27, 2025

Uh oh!

edschema commented Jun 27, 2025

Uh oh!

Uh oh!

openedx-webhooks commented Jun 22, 2025 •

edited

Loading

codecov bot commented Jun 24, 2025 •

edited

Loading

ChrisChV left a comment •

edited

Loading