Skip to content
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

8829/fix/redesign sort UI #9489

Merged
merged 38 commits into from
Sep 29, 2024

Conversation

aarasawa
Copy link
Contributor

Closes #8829

Refactor UI for the sorting option selectors in the search bar. Changed from radio/individual buttons into a dropdown menu.

Technical

Still need to work on the styling of the dropdown menu.

Testing

Screenshot

Stakeholders

@mekarpeles
Copy link
Member

mekarpeles commented Jun 27, 2024

@merwhite11 I left some feedback for @aarasawa on the review -- maybe you two can collaborate to try this out using the details component? It could be useful to prototype as a codepen, similar to @SivanC's for the unified read button: https://codepen.io/sivanc/pen/VwOpXwq?editors=1100

@mekarpeles mekarpeles added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Jun 27, 2024
@github-actions github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Jun 29, 2024
@aarasawa
Copy link
Contributor Author

aarasawa commented Jul 8, 2024

I thought I would push the updates that I had with implementing using details tag as @mekarpeles recommended. It also got rid of the use of manual spacing.

Need help with CSS. The CSS that the span element in the HTML uses is in legacy.less. I have some styling that I attempted to try to use however they never rendered, even when I tried moving styling for details into its own CSS .less file.

Here is the example CSS I wanted to try: link

@merwhite11
Copy link
Collaborator

@mekarpeles @aarasawa
Alex and I are meeting to go over questions / css for this.

@aarasawa
Copy link
Contributor Author

Current problems with overriding styling on anchor tags within the drop down menu. I am pushing what we have so that hopefully @rebecca-shoptaw can help when they can.

Copy link
Collaborator

@rebecca-shoptaw rebecca-shoptaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gave it a look over, see the feedback below re: how to switch out the colors and get the tests passing -- and the style override should be no problem at all:

I believe it could be as simple as changing a to a:link in the css, this is what the overriding CSS uses and may be how it is one-upping your code as is. This way your code ideally would take precedence and you wouldn't need the !important

static/css/components/sort-dropper.less Outdated Show resolved Hide resolved
static/css/components/sort-dropper.less Outdated Show resolved Hide resolved
static/css/components/sort-dropper.less Outdated Show resolved Hide resolved
static/css/components/sort-dropper.less Outdated Show resolved Hide resolved
static/css/components/sort-dropper.less Outdated Show resolved Hide resolved
static/css/components/sort-dropper.less Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.05%. Comparing base (ce16a79) to head (370c08f).
Report is 453 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9489      +/-   ##
==========================================
+ Coverage   16.06%   17.05%   +0.99%     
==========================================
  Files          90       89       -1     
  Lines        4769     4725      -44     
  Branches      832      829       -3     
==========================================
+ Hits          766      806      +40     
+ Misses       3480     3409      -71     
+ Partials      523      510      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mekarpeles mekarpeles assigned mekarpeles and unassigned merwhite11 Sep 20, 2024
@mekarpeles mekarpeles added the Priority: 1 Do this week, receiving emails, time sensitive, . [managed] label Sep 20, 2024
@mekarpeles
Copy link
Member

@aarasawa this is looking fantastic on /search and lists and is ready to go, however the sorting UI looks a bit broken for author pages:

e.g.
Screenshot 2024-09-26 at 10 04 25 AM

@aarasawa
Copy link
Contributor Author

@aarasawa this is looking fantastic on /search and lists and is ready to go, however the sorting UI looks a bit broken for author pages:

e.g. Screenshot 2024-09-26 at 10 04 25 AM

Oo, I'll take a look at that, thank you for pointing it out @mekarpeles

@cdrini
Copy link
Collaborator

cdrini commented Sep 27, 2024

Heads up taking this off testing.openlibrary.org to test another PR 👍

@aarasawa
Copy link
Contributor Author

Something odd that happens is that the template seems to render incorrectly as pointed out in the authors page. #9489 (comment)

However, when I go up in the chain and add an empty div in type>author>view.html it fixes the problem.
image (1)
image

@mekarpeles mekarpeles merged commit 8bc0338 into internetarchive:master Sep 29, 2024
4 checks passed
@aarasawa aarasawa deleted the 8829/fix/redesign-sort-ui branch September 30, 2024 04:40
DanielleInkster pushed a commit to DanielleInkster/openlibrary that referenced this pull request Oct 1, 2024
* Redesigned UI of sort options into dropdown menu in /search
* Reimplemented dropdown menu closer to cta-btn and added CSS component
* Create sort-dropper.less
* Added i18n internationalization support for dropdown text and CSS changes
* Added CSS style revisions to better understand sort option selection
* Swapped dropdown menu caret icons to stay consistent with Want to Read dropdown

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: rebecca shoptaw <[email protected]>
Co-authored-by: Mek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 1 Do this week, receiving emails, time sensitive, . [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search Page: Redesign "Sort" options UI
6 participants