-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Redesign search box on search pages #9146
base: master
Are you sure you want to change the base?
Conversation
@0simoo please see the comment here #9086 (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.
@0simoo
A few small comments, but the main thing is that from the comment here @mekarpeles wants you to use the magnifying glass instead of the word "search"
One you do that and add these other small comments I think we'll be good to go!
I made a few more changes (sorry for using so many commits), let me know what 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.
@0simoo good work on this one! A few comments.
- Can you make it so the font size is uniform across all the pages?
- Can you make the height below the search bar on each space equal (see video)
- Can you make the touch target bigger (maybe even square?) for the search icon? (see pictures)
- Lets make the background color white (design folks might have different opinions once they see it).
PS: once you do these I'll ping the design team for comments 👍
search.Open.Library.-.10.May.2024.mp4
Small touch target | Larger touch target |
---|---|
…reased touch target of search icon
@0simoo are you still working on this or are your latest changes ready for review? |
@RayBB I think I've made all of the changes except for the height issue. I've gotten all the tabs to be the same height except for the Authors search. It look like a div called I can continue working on this if you'd like, but I've started my internship recently so my progress might be slower. |
@0simoo thanks your changes are working great. |
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.
Looks really good just one small change! I'm gonna mark as needing staff now so as soon as you push that fix they can check it out.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9146 +/- ##
=======================================
Coverage 16.20% 16.20%
=======================================
Files 91 91
Lines 4740 4740
Branches 822 822
=======================================
Hits 768 768
Misses 3460 3460
Partials 512 512 ☔ View full report in Codecov by Sentry. |
Howdy! Put this up on testing. It's looking great! A few fixes:
The design is looking fantastic though, can't wait to start using this everywhere! Nice work! |
max-width: 93% !important; | ||
} | ||
|
||
.search-btn { |
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.
This doesn't really make sense in this file ; this css file is just about the searchbox
; this is kind of just a random other button :P This should be based on our exisitng cta-btn, which correctly handles things like font size, hover states etc.
See where .cta-btn
is defined, and add a new one, cta-btn--search
. Then to the element, add class="cta-btn cta-btn--search"
I think I implemented all the changes. Let me know if there are any issues! |
@0simoo I'll have some time to review this in the next few days. Could you provide some screenshots of what it now looks like on desktop and mobile? |
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.
I have a few more suggestions here that I'm going to merge in and deploy to testing and then look again so we can do another round of feedback 👍
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.
I think it looks pretty great now. Latest version is on testing.
Lets have @cdrini take another look..
@0simoo good work on this so far! |
Closes #9086
Feature: New frontend change to search UI. Replaced old search text input and button with new design.
Technical
This fix also tries to solve a visual bug where the space underneath the searchboxes were different in height (varied in height from books search, subjects search, etc.)
This is mostly fixed, except for the Authors search due to a div called contentMeta that is taking up space under the searchbox.
Testing
Changes can be seen at https://openlibrary.org/search
The new design should be present for all 6 types of searches (books, authors, search inside, subjects, lists, advanced search)
Screenshot
Old search button:
New search button:
New search button on mobile:
Stakeholders
@jimchamp @RayBB @mekarpeles