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

Changed CSS so that the audiobook text does not overlap with the icon #10112

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

LouisSim
Copy link

@LouisSim LouisSim commented Dec 4, 2024

Closes #8899

Fix: Fixes issue with the text overlapping in mobile

Technical

Changed css as directed in #9066, this seems to work to resize the text so that there is no issue with the overlap with the icon.

Testing

docker compose up
// For running site
docker compose exec -e PYTHONPATH=. web bash -c "./scripts/copydocs.py --search 'author_key:OL9411725A' --search-limit 100"
// Adding data
http://localhost:8080/works/OL36891831W/
Start changing screen width

Screenshot

image
image
image

Stakeholders

@cdrini
@RayBB

@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.42%. Comparing base (347bff9) to head (eeea04b).
Report is 128 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10112      +/-   ##
==========================================
+ Coverage   17.12%   17.42%   +0.29%     
==========================================
  Files          89       89              
  Lines        4752     4799      +47     
  Branches      831      847      +16     
==========================================
+ Hits          814      836      +22     
- Misses       3428     3444      +16     
- Partials      510      519       +9     

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

Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Awesome, thank you @LouisSim ! I think we might have a regression with the button on search pages here 😁
image

@LouisSim
Copy link
Author

LouisSim commented Dec 4, 2024

How can I replicate that issue, I am not seeing any problems on the search page but I might be looking in the wrong place

@RayBB
Copy link
Collaborator

RayBB commented Dec 4, 2024

@LouisSim do you see any books with the audio option in your search result? Also have you run make css (in docker) to update the css?

@LouisSim
Copy link
Author

LouisSim commented Dec 4, 2024

Yeah, now I see it, the books I was looking at didn't have the audio option. Thanks!

@LouisSim
Copy link
Author

LouisSim commented Dec 4, 2024

I updated the styling so it only applies to buttons in the carousel-section. This fixes the regression issue. I also checked around to make sure no other buttons were affected by the change. Let me know if there’s anything else I should look into!

image

@RayBB
Copy link
Collaborator

RayBB commented Dec 6, 2024

@LouisSim I'm struggling with my local environment right now but will test with Drini next week.

Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Nice work @LouisSim ! One more fix I'll put on testing, and then we should be good to go!

static/css/components/buttonCta.less Outdated Show resolved Hide resolved
@LouisSim
Copy link
Author

Great, thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Waiting Review/Merge from Staff
Development

Successfully merging this pull request may close these issues.

Trusted book provider "audiobook" link in carousel on mobile has text overlap with external icon
4 participants