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

Copy/Paste Option for adding Book Covers resolving #9993 #10101

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

Conversation

Spaarsh
Copy link
Contributor

@Spaarsh Spaarsh commented Nov 30, 2024

Closes #9993

feature

Technical

Changes are limited to .html and .js files only. It is still a bit buggy on the front end side. A user may be shown 'No Image Selected' even if that is not the case. But the submission will be successfully done nonetheless.

Testing

  1. Go to any works page.
  2. Either click on the 'Add Another Issue' or try to edit any existing edition and skip to step 4.
  3. Add the details for a dummy issue and proceed.
  4. Click on 'Manage Covers' button.
  5. If the add.html page now features a paste option which successfully adds the new cover, the code is successful.

Screenshot

image
image
image

Stakeholders

@RayBB @cdrini @mekarpeles @Freso

@Spaarsh
Copy link
Contributor Author

Spaarsh commented Dec 3, 2024

@cdrini Are any changed required? Waiting for your review!

@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 0% with 43 lines in your changes missing coverage. Please review.

Project coverage is 17.26%. Comparing base (347bff9) to head (504b4b4).
Report is 131 commits behind head on master.

Files with missing lines Patch % Lines
openlibrary/plugins/openlibrary/js/covers.js 0.00% 32 Missing and 5 partials ⚠️
openlibrary/plugins/openlibrary/js/index.js 0.00% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10101      +/-   ##
==========================================
+ Coverage   17.12%   17.26%   +0.13%     
==========================================
  Files          89       89              
  Lines        4752     4841      +89     
  Branches      831      853      +22     
==========================================
+ Hits          814      836      +22     
- Misses       3428     3480      +52     
- Partials      510      525      +15     

☔ 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.

Nice work @Spaarsh this looks like a great start! A few code fixes from a quick first pass.

Also for the UI: Place the image inside the "paste image" grey box ; just above the "Use this image" button. Like so:

image

We use this UI for one of the other grey boxes, so see if you can re-use the CSS there.

openlibrary/plugins/openlibrary/js/index.js Outdated Show resolved Hide resolved
openlibrary/templates/covers/add.html Outdated Show resolved Hide resolved
openlibrary/plugins/openlibrary/js/covers.js Outdated Show resolved Hide resolved
try {
const clipboardItems = await navigator.clipboard.read();
for (const item of clipboardItems) {
if (item.types.includes('image/png')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah there's a bug here; since there can be multiple items in the clipboardItems, you'll want to skip any non image types, not error. Eg

if (!item.types.includes('image/png')) {
    continue;
}

And also check for jpg images as well, since those will work too!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes done. Additionally, initially I had included only png support because clipboards store all copied image formats in .png irrespective of the initial image format. However after a little digging after your suggestion, I found out that in Windows, users can changes their settings to store images in the jpg format. I may have to dig deeper into this, but I do not see any harm in adding the support right now.

openlibrary/plugins/openlibrary/js/covers.js Show resolved Hide resolved
openlibrary/templates/covers/add.html Outdated Show resolved Hide resolved
@cdrini cdrini added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Dec 4, 2024
@github-actions github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Dec 4, 2024
@Spaarsh
Copy link
Contributor Author

Spaarsh commented Dec 4, 2024

This is the updated UI:
image

All the other recommended changes have been made as well. There is another issue now, the add.html page is not scrollable and hence, the bottom most part of the page is inaccesible. Should I make the necessary changes in the CSS as well to address that?

@Spaarsh
Copy link
Contributor Author

Spaarsh commented Dec 15, 2024

@cdrini any updates on this?

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.

Upload covers via copy/paste
3 participants