Content Search Modal: Filters [FC-0040] #918
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @bradenmacdonald! 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. |
bradenmacdonald
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #918 +/- ##
==========================================
- Coverage 92.00% 91.96% -0.05%
==========================================
Files 612 586 -26
Lines 10746 10351 -395
Branches 2305 2263 -42
==========================================
- Hits 9887 9519 -368
+ Misses 830 805 -25
+ Partials 29 27 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hi @bradenmacdonald! I quickly looked at the code and commented about small things missed from the other PR.
Tomorrow, I'll test it and do a proper review.
| const studioBaseUrl = getConfig().STUDIO_BASE_URL; | ||
| const meiliSearchEnabled = [true, 'true'].includes(getConfig().MEILISEARCH_ENABLED); |
There was a problem hiding this comment.
LGTM @bradenmacdonald! 👍
- I tested this running using my tutor stack
- I read through the code
- I checked for accessibility issues
- There is a minor problem here. You can't leave the
CheckboxSetif you navigate using tabs. The problem is probably in the Paragon component.
- There is a minor problem here. You can't leave the
- Includes documentation
|
Thanks @rpenido! I'll look into the a11y issue you found:
|
|
@bradenmacdonald Could you rebase this? I'll review it tomorrow. |
|
@xitij2000 Done. Thanks! |
|
Test failure is some temporary issue with codecov; should go away later. |
bradenmacdonald
force-pushed
the
braden/filter-ui
branch
from
273e0b0 to
881c8ec
Compare
|
Discussion of the failing codecov check: https://openedx.slack.com/archives/C04BM6YC7A6/p1712684355941999?thread_ts=1711620349.847079&cid=C04BM6YC7A6 Seems like we just have to wait as it's a GitHub-wide rate limiting problem. |
bradenmacdonald
force-pushed
the
braden/filter-ui
branch
from
881c8ec to
650b733
Compare
|
@xitij2000 I got a green build. Could you please "Squash and Merge"? :) |
|
@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
@rpenido Just following up on this: I think that's expected behavior. When the menu is open, the tab only moves among the items in the menu. To leave the menu, you press ESC and then you can TAB through the rest of the controls in the modal. |
As of #918 , the content search only allows filtering the results by one tag at a time, which is a limitation of Instantsearch. So with this change, usage of Instantsearch + instant-meilisearch has been replaced with direct usage of Meilisearch. Not only does this simplify the code and make our MFE bundle size smaller, but it allows us much more control over how the tags filtering works, so that we can implement searching by multiple tags. Trying to modify Instantsearch to do that was too difficult, given the complexity of its codebase. Related ticket: openedx/modular-learning#201
Description
Implementation of openedx/modular-learning#201 .
Video of this feature:
Studio.Search.Demo.mov
Goal was to implement the top half of this:
Note that some things
Other information
Goal: Redwood
TODO
<Stats/>, localize itTODO in a follow-up PR:
Private ref: FAL-3696