Conversation
abeglova
force-pushed
the
ab/new-search
branch
from
5502f36 to
062a393
Compare
|
Netlify Deployments: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
| Device | URL |
|---|---|
| mobile | https://ocw-hugo-themes-www-pr-1313--ocw-next.netlify.app/ |
| Device | URL |
|---|---|
| mobile | https://ocw-hugo-themes-www-pr-1313--ocw-next.netlify.app/search/ |
| Device | URL |
|---|---|
| mobile | https://ocw-hugo-themes-course-v2-pr-1313--ocw-next.netlify.app/ |
Not what you expected? Are your scores flaky? GitHub runners could be the cause.
Try running on Foo instead
| ) | ||
| } | ||
|
|
||
| const propsAreEqual = (_prevProps: Props, nextProps: Props) => { |
There was a problem hiding this comment.
We can't use this anymore since the aggregation is no longer nested in the bucket key - but the facets don't appear and disappear so I think the issue has been fixed elsewhere
www/assets/js/lib/constants.ts
Outdated
| course_feature_tags: RESOURCE_TYPES, | ||
| resource_type: RESOURCE_TYPES | ||
| resource_type: ["course"], | ||
| department: Object.keys(departments), |
There was a problem hiding this comment.
We should have a followup pr to get all the options from the generated api documentation
|
|
||
| export const searchResultToLearningResource = ( | ||
| result: LearningResourceResult | ||
| export const courseSearchResultToLearningResource = ( |
There was a problem hiding this comment.
We should have a follow up pr to get rid of the LearningResource type defined in ocw-hugo-themes and update the cards in SearchResult.tsx to expect the objects returned from the search api directly and update courseJSONToLearningResource and resourceJSONToLearningResource to convert ocw json to CourseResource and ContentFile from the open api. It's a little messy since LearningResource can currently be either a course or a resource but the parameters in CourseResource and ContentFile are not the same
ChristopherChudzicki
left a comment
ChristopherChudzicki
left a comment
There was a problem hiding this comment.
Overall, this is working very well. I've tested:
- filtering
- textual searches
- resource files & courses
- browser navigation (forward/back)
I still need to look at the actual course-search-utils changes.
Changes
-
levelsMap: Right nowlevelsMapis constructed fromLearningResourcesListLevelEnum. This products bad values forhigh_schoolandnoncredit, namely "HighSchool" and "NonCredit" (no space / hyphen).- Maybe just use an explicit mapping like we do for departments?
-
Course links: In the course theme, there are links to the search page using old "Department" and "Level" values
- department appears here: https://ocw.mit.edu/courses/18-102-introduction-to-functional-analysis-spring-2021
- level appears in the course-info box: https://ocw.mit.edu/courses/18-102-introduction-to-functional-analysis-spring-2021/video_galleries/lecture-videos/
In slack we discussed making the search page with both the old and new department/level values, at least temporarily. I think that's reasonable to ease the eventual deployment.
Further checking
Additionally, I think we should:
- Set
CORS_ALLOWED_ORIGIN_REGEXES="ocw-hugo-themes-www-pr-\d+--ocw-next\.netlify"on MIT Open Prod (if we agree that's OK) - Update the netlify workflow to use MIT Open Prod
That way we can share a version of this that uses the prod search for easier comparison with live OCW.
| import { emptyOrNil, isDoubleQuoted } from "../lib/util" | ||
| import { FacetManifest, LearningResourceResult } from "../LearningResources" | ||
| import { FacetManifest } from "../LearningResources" | ||
| import { CourseResource, ContentFile } from "../open_api_generated/api" |
There was a problem hiding this comment.
Here and elsewhere that you use types only, I would do
| import { CourseResource, ContentFile } from "../open_api_generated/api" | |
| import type { CourseResource, ContentFile } from "../open_api_generated/api" |
which makes intent clear, and also 100% guarantees there is no impact on runtime.
|
Also: With the |
abeglova
force-pushed
the
ab/new-search
branch
from
b8291a5 to
5cd048a
Compare
There was a problem hiding this comment.
URLS like http://localhost:3000?d=Mechanical%20Engineering&d=Mathematics&l=High%20School are working great 👍 :
| updateEndpoint(nextResourceFilterState) | ||
| }, | ||
| [toggleFacets, activeFacets] | ||
| [toggleFacets, activeFacets, endpoint, updateEndpoint, sort, updateSort] |
There was a problem hiding this comment.
I would remove the sort dependency like linter says.
abeglova
force-pushed
the
ab/new-search
branch
from
963e8b5 to
26ab8d1
Compare
* move filters to course search util
|
@ChristopherChudzicki @abeglova what should we do with this PR? I'm not really clear on what it does. |
|
Is it worth keeping this PR open, or is it hopelessly out of date? |
3 participants
What are the relevant tickets?
closes #1294
Description (What does it do?)
This pr updates the search to use the new open apis.
It uses the changes to course-search utils from mitodl/course-search-utils#64
How can this be tested?
Run mit-open
Set
COURSE_SEARCH_API_URL=http://localhost:8063/api/v1/learning_resources_search/
CONTENT_FILE_SEARCH_API_URL=http://localhost:8063/api/v1/content_file_search/
Run
yarn start wwwGo to http://localhost:3000/search/ and click around. Everything should work as expected. The urls will be a little different than the existing ui