human-tools / toolbox-web

Simple Secure and Private Tools for everyday humans
https://humantools.io
22 stars 3 forks source link

Combine pdf doesn't seem to be working as expected #7

Closed Fahme closed 3 years ago

Fahme commented 3 years ago

Hey,

Thanks for your work on this i've always searched on the web for tools to help me edit pdf files easily without the hustle of uploading my files to servers and allowing me to get simple tasks done as fast as possible, and i believe these tools are just a great helping hand, also i'm super excited for the sign pdf feature as i do a-lot of pdf signing digitally :)

The current bug i'm having with combine pdf tool is that i try to upload multiple pdf files to merge them but only one file seems to be uploaded and previous ones are getting removed for some reason.

I'm expecting to be able to upload multiple pdf files and have option to sort them and maybe delete some pages that i don't want to show on my pdf file.

Probably a nice to have feature also is i can click on a pdf page and it opens in a full view.

if you needed help maybe i can send a PR to fix the bug and introduce the delete feature.

mkhatib commented 3 years ago

@Fahme thanks! Glad to see these tools are helpful and thanks for the bug report!

Yes, currently the tool expects all the files to be merged to be uploaded at the same time (dragged or selected in the file dialog), so when you select or drag a new file separately it clears any older selected files. I agree this is not the wanted behavior and we probably want to expose a different way for the user to start over and that the behavior should be to append any new pdfs pages to the currently being edited one.

We welcome PRs! Please let us know if you need any guidance or if docs are not up to date.

Quick-previewing a specific page sounds like a good feature to add too (we have thought also about adding a slider to adjust the size of the page previews thumbs).

Removing pages is a great feature to add too

Fahme commented 3 years ago

I'm thinking the flow for creating and appending new pdf files would be :

what do you think ? @mkhatib

mkhatib commented 3 years ago

That sounds good to me!

mkhatib commented 3 years ago

@Fahme, @ahmgeek and I just paired and fixed the issue with new files selected erasing prior work! We didn't add a confirmation dialog and always assume that the human wants to append new files. We can add a more clear messaging (or a confirmation dialog) in another change. But humans can now just refresh the page if they want to start over.

@ahmgeek splendid work!

mkhatib commented 3 years ago

@Fahme it would be nice to open issues for the other improvements you mentioned in the original text if you're up for it so we can keep better track of them!

Fahme commented 3 years ago

Great stuff, thanks for you work but still there's a couple of implementation bugs in @ahmgeek PR.

Ideally i don't think the user want to lose the files order when new documents get added, currently if i add a new document to an existing list a re-render happens causing all of my files to return to their original order (if i did any changing to my files order)

After spending a couple of minutes reading through the PR and useSortable module i think a few changes could be made to the useSortable API to make this work. Also that's what made me take longer time to send the fix on my end.

@mkhatib

mkhatib commented 3 years ago

Good catch @Fahme ! We're working on a fix rn! I don't think this is a useSortable problem and should be the responsibility of the consumer of that library to not override an existing order list.

mkhatib commented 3 years ago

Also, apologies if we duplicated your work! I wasn't sure you were still working on it. Maybe for future you can send a Draft PR as you're working so we know not to duplicate the work.

mkhatib commented 3 years ago

@Fahme hopefully the new fix does it! Let us know!