-
Notifications
You must be signed in to change notification settings - Fork 0
feat: enable to select collections as components in problems picker #2
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
base: ednx-release/teak.ceibal
Are you sure you want to change the base?
feat: enable to select collections as components in problems picker #2
Conversation
MaferMazu
left a comment
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.
The functionality works fine, thanks. My only concern is the efficiency of selecting elements from the collections (because the number of problems and collections will increase). We can handle documentation that explains the client's limitations, but if we can find a way to make it more efficient, that would be great.
|
|
||
| // Select another component | ||
| fireEvent.click(screen.queryAllByRole('button', { name: 'Select' })[1]); | ||
| fireEvent.click(screen.queryAllByRole('button', { name: 'Select' })[7]); |
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.
Can you add more information on why we need this change? Something simple.
I won't argue about the test change, but it could be valuable to track the why.
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.
@MaferMazu, this PR allows selecting collections too, due to it, once in the test it is queried screen.queryAllByRole('button', { name: 'Select' }) which returns an array bigger than before (before contained 3 items, now it includes 9 items counting the collections) and taking into account that in the library-search.json the collections are before the components (and rendered before the components), the index of the components changed from 1 to 7 (with 6 collections in between)
I could change the library-search.json items order and preserve the test with index 1, what do you think?
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.
Yes, you can change the JSON order, or in this case, leave it as it is; at least we have it documented in some way. Another thing you could do is use a specific id instead of the index, or filter only the components (but not necessary for this PR).
c9c43bd to
d232be1
Compare
d232be1 to
b6908d1
Compare
MaferMazu
left a comment
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.
@bra-i-am thanks a lot for this PR, it worked as expected!
|
@bra-i-am I just realized that with big numbers, we have an issue when selecting elements. Can you guide me on how to fix that, or, if there isn't much time, help me fix it? Screencast.from.2026-01-22.17-57-12.webm |
9fd69a0 to
7fc902e
Compare
7fc902e to
d0cb659
Compare
MaferMazu
left a comment
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 for all the changes, @bra-i-am. I'll use it on stage to finish testing.
I noticed we had a test comment regarding nextOffset and getNextPageParam. If we are not going to solve it, can we leave a comment in the PR explaining why, or can we solve it?
|
To add information to this PR, I asked @bra-i-am to increase the number of components shown per page in libraries from 20 to 100. This is intended to reduce the time spent searching between pages when bringing all the elements from the collections. But it is because, in our case, for Ceibal, we expect to use that amount of components or more. That change, for now, is not part of the upstream change openedx#2824. I added to the Ceibal product documentation that it would be desirable to add an upstream change that allows configuring that limit. |
|
@bra-i-am I tested in stage, and it worked as expected. Thanks a lot for solving the issues. After solving the nextOffset issue, or at least having a comment regarding that, we can merge this. |
This pull request refactors and optimizes how collections and their components are indexed and filtered in the library authoring UI. The main improvements are the introduction of a centralized collection indexing context for O(1) lookups, frontend filtering for the Collections tab to enable more flexible workflows, and associated updates to component interfaces and context providers. These changes improve performance and maintainability, especially when working with large sets of collections and components.
Collection indexing and context improvements:
CollectionIndexProviderand related hooks/types inComponentPickerContext.tsxto precompute and share collection/component relationships and sizes across the UI, avoiding repeated computation and improving performance. This includes new interfaces and a context for accessing the precomputed data. [1] [2]LibraryAuthoringPageandLibraryCollectionPage) withCollectionIndexProviderto ensure all children have access to the precomputed collection index data. [1] [2] [3] [4] [5] [6]Filtering and content display logic:
LibraryContent.tsx, ensuring all components remain available in the search hits for selection workflows. Adjusted the filtering logic inLibraryAuthoringPage.tsxaccordingly. [1] [2] [3]LibraryCollectionComponents.tsxto use the defaultLibraryContent(with frontend filtering) instead of passing a collections-specific content type. [1] [2]Type and interface enhancements:
Test data updates:
block_idfields to mock collection data inlibrary-search.jsonto better reflect the updated data structures and support new indexing logic. [1] [2] [3] [4] [5]How to test
Make sure you have a collection in your Libraries ( http://apps.local.openedx.io:2001/authoring/libraries ) with questions in it
Settings > Advanced Settingsand add"examquestionbank"to the list ofAdvanced module listAdvancedin the component selector and add anExam Question BankAdd Problemsand select a collection; it should be selected and deselected properly, and once you clickAdd Selected Components, they should be displayed in the xblock as will be shown in the following screencastScreencast
Screencast.from.09-01-26.14.40.43.webm