Skip to content

Fixed extra white space issue on book preview modals#10142

Merged
mekarpeles merged 7 commits intointernetarchive:masterfrom
DachiCharkviani:master
Dec 30, 2024
Merged

Fixed extra white space issue on book preview modals#10142
mekarpeles merged 7 commits intointernetarchive:masterfrom
DachiCharkviani:master

Conversation

@DachiCharkviani
Copy link
Contributor

@DachiCharkviani DachiCharkviani commented Dec 11, 2024

Closes #10095

Worked with @graissov and @khiyami on this issue.

Fix/Refactor excess whitespace on book review pop-ups.

Technical

Testing

Go to this page and click preview(or any other book with preview): http://localhost:8080/works/OL16086453W/Pioneers

Screenshot

Before:
Screenshot 2024-12-11 at 9 02 59 AM

After:
Screenshot 2024-12-11 at 9 02 33 AM

Stakeholders

@RayBB

@DachiCharkviani DachiCharkviani marked this pull request as draft December 11, 2024 06:15
@DachiCharkviani DachiCharkviani marked this pull request as ready for review December 11, 2024 06:15
@DachiCharkviani
Copy link
Contributor Author

@RayBB Could you please review this PR. I believe I am done with issue #10095

@RayBB
Copy link
Collaborator

RayBB commented Dec 11, 2024

@DachiCharkviani my local environment to test changes is broken and I'm working on fixing it.
I hope @cdrini can check this out this week.

@DachiCharkviani
Copy link
Contributor Author

Hello, @RayBB Thank you for your answer. No rush, but if it is processed as soon as possible that will greatly benefit me since I have to show in my college class that I have successful merge in open source code on git for extra credit. Once, again thanks.

@cdrini Waiting for your approval then! All the best.

Dachi and others added 2 commits December 12, 2024 01:29
@DachiCharkviani
Copy link
Contributor Author

Hello, @RayBB, @cdrini , if you could please review the merge for this issue, I would be very thankful for that. I will get extra for the potential PR merge, and save my grade. Thank you in advance. I believe the issue is solved with this code.

Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

Please remove all changes not related to the modal.

@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.44%. Comparing base (347bff9) to head (93ae194).
Report is 152 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10142      +/-   ##
==========================================
+ Coverage   17.12%   17.44%   +0.31%     
==========================================
  Files          89       89              
  Lines        4752     4792      +40     
  Branches      831      848      +17     
==========================================
+ Hits          814      836      +22     
- Misses       3428     3436       +8     
- Partials      510      520      +10     

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

@DachiCharkviani
Copy link
Contributor Author

@RayBB done!

@DachiCharkviani
Copy link
Contributor Author

@RayBB Could you please respond for the merge? If everything is on point and satisfies the requirements I would like to proceed as soon as possible because deadline for the credit is today night.

@DachiCharkviani
Copy link
Contributor Author

@cdrini @RayBB I believe the merge is ready. Could you please review?

Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

@DachiCharkviani great work!

I tested this on desktop and on mobile and it works great.
One small change I pushed up is to remove some unused css after you removed the class.
Removing the floater and the iframe-container class were both needed.

Now we just need one of the staff to review/merge it.
Probably @cdrini but I think he might be on vacation now.

@RayBB RayBB added the Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. label Dec 17, 2024
@mekarpeles mekarpeles self-assigned this Dec 23, 2024
@mekarpeles mekarpeles merged commit b73fe2b into internetarchive:master Dec 30, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Weird white gap at bottom of preview dialog

4 participants