-
Notifications
You must be signed in to change notification settings - Fork 208
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: integrate openedx search api with existing CoursewareSearch.jsx #1433
feat: integrate openedx search api with existing CoursewareSearch.jsx #1433
Conversation
Thanks for the pull request, @qasimgulzar! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
@qasimgulzar Please provide full, detailed instructions for how to test this, including links to all other required PRs and configuration settings. Personally, I would also really like to see what it looks like when the studio search (in course-authoring) is converted to use this, as it's a far more complex use case than the very simple course search UX here. |
OSPR management note: Product review in progress via openedx/platform-roadmap#378. |
4db3961
to
6845cd1
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.
For new code like this, I would strongly prefer we load the search engine config and results using React Query like this rather than using Redux. See OEP-67.
@@ -7,5 +7,6 @@ config.resolve.alias = { | |||
...config.resolve.alias, | |||
'@src': path.resolve(__dirname, 'src'), | |||
}; | |||
config.externalsType = 'script'; |
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.
What is this for?
Note to self: to test this PR, we have to enable the |
OSPR management note: Marking this as blocked based on this update from @ormsbee over on the product proposal ticket. |
Hey @qasimgulzar, could you please check if this PR is still needed now that @regisb created another PR (openedx/edx-platform#35650) for addressing openedx/platform-roadmap#378? |
Let's close this PR for the moment. The interface that it proposes is different from my PR openedx/edx-platform#35650 because it connects to Meilisearch directly, but we can always re-open it in the future if we decide that we want to go with this approach. |
Thanks @regisb! |
Under this PR I have used openedx-search-api to enable courseware search feature.
Screen.Recording.2024-08-08.at.11.23.33.PM.mov