-
Notifications
You must be signed in to change notification settings - Fork 3
Add a Search Page #618
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
Add a Search Page #618
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #618 +/- ##
==========================================
+ Coverage 74.81% 75.00% +0.18%
==========================================
Files 245 247 +2
Lines 11204 11308 +104
Branches 1947 1962 +15
==========================================
+ Hits 8382 8481 +99
- Misses 2648 2653 +5
Partials 174 174 ☔ View full report in Codecov by Sentry. |
152c775 to
c657749
Compare
We've updated react router, so we can use theirs.
c657749 to
602ab60
Compare
|
This really does work very nicely. The only thing I noticed while testing the UI was that the search clear doesn't reset results (or clear the URL search param). There are couple of small layout issues for mobile / smaller screen width. I appreciate we are working off designs for desktop. Do we have plans for mobile at this point? |
shanbady
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.
works like it should 👍
I think we should try to fix this in this PR if it's an issue. We can deal with perfecting mobile later. |
| accent-color: ${({ theme }) => theme.palette.primary.main}; | ||
| } | ||
|
|
||
| .filter-section-main-title { |
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.
It would be good to find a solution that doesn't depend on classnames in dependencies. I do see the dilemma - how to write course-search-utils that are customizable, but still largely hold their layout and base styles without having to pass so many components in that lose code reuse.
If course-search-utils also used styled components, we could provide them to the css template literal for interpolation (we discussed that a bit the other day). The benefit would be that course-search-utils API is defined by its exports and not additionally by its classnames. Not one for this PR, but food for thought.
Retested after @ChristopherChudzicki's last change - works perfectly! |
What are the relevant tickets?
Closes #490
Description (What does it do?)
This PR adds a search page. We don't have final designs for this, but I used https://www.figma.com/proto/bRb19HaucO5loHGSU81nFG/MIT_Prototype?page-id=0%3A1&type=design&node-id=9-6102&viewport=4023%2C1145%2C0.25&t=6VyDzTtHVzsZ3Nyv-1&scaling=scale-down&starting-point-node-id=48%3A2446&show-proto-sidebar=1 as inspiration.
Note: After discussing with @Ferdi, only "Topics" facet is supported for now (and "Resource Type" via the tabs).
Screenshots (if appropriate):
How can this be tested?