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

Fixes to lists-showcase styling/code #9412

Merged
merged 6 commits into from
Jun 10, 2024
Merged

Conversation

cdrini
Copy link
Collaborator

@cdrini cdrini commented Jun 7, 2024

Closes #7704 (Addendum)

Finished some of the design details in figma + DRYed up some of the code.

Technical

Code:

  • Used BEM notation correctly; the code was already structured BEM-like, but was using - instead of __ to denote child relationship
  • The HTML had a few unnecessary nesting/divs; dropped those
  • I18n all the strings
  • Fix some bugs in the code (eg default parameters cannot be lists)
  • Used larger covers since they're displayed larger than the -S
  • Used less position: absolute in the list-card
  • Used CSS to truncate title, not manual limiting to 16 characters

UI:

  • I removed the gradient at the end ; that's a pretty common UI affordance for scrolling; we should bring back now that scrolling is enabled, but it takes a bit of time since you have to make it appear/disappear as necessary.
  • Added a "see all" button at end now that it's scrollable
  • The covers ended up a touch easier staggered; I like the effect so kept it in

Testing

Tested ~latest Chrome, FF, and Safari (iPad).

Screenshot

image

Stakeholders

@mekarpeles

@cdrini cdrini added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Jun 7, 2024
@cdrini cdrini marked this pull request as ready for review June 7, 2024 23:04
@cdrini cdrini changed the title Fixes to lists-showcase styling Jun 7, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 16.12%. Comparing base (c8203dd) to head (f0b3908).
Report is 95 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9412      +/-   ##
==========================================
- Coverage   16.13%   16.12%   -0.02%     
==========================================
  Files          92       90       -2     
  Lines        4766     4758       -8     
  Branches      827      828       +1     
==========================================
- Hits          769      767       -2     
+ Misses       3480     3471       -9     
- Partials      517      520       +3     

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

@cdrini
Copy link
Collaborator Author

cdrini commented Jun 10, 2024

Auto-merging this one as an addendum to #8878 , since some feedback got lost during the CR process. Usually wouldn't auto-merge such a large one, but need the style fixes to go out before the deploy. I QA'd on testing on a few different devices/via browserstack. Assigning to you @mekarpeles for a post-merge review 👍

@cdrini cdrini merged commit 0f5474e into internetarchive:master Jun 10, 2024
9 checks passed
@cdrini cdrini deleted the fix/styling branch June 10, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing
3 participants