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

Use Xhr to upload and show progress #116

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

Conversation

Lex-2008
Copy link

@Lex-2008 Lex-2008 commented Dec 23, 2022

I saw that you had some styles for .progress .meter but didn't find any elements created so (ab)used the size field.

Also, "placeholder" property of Entry is used as a flag to show that relevant file is being uploaded. It works for now, but if someone else will want to use it for something else in the future, there might be a conflict.

Also, I tried to reject promise and process it inside a try{...} catch block, but didn't succeed in providing type hints that would satisfy VS Code, so instead of that, XHRPutFile now returns a promise with XMLHttpRequest object, which is verified outside of that function.

@dom111
Copy link
Owner

dom111 commented Dec 26, 2022

This is great! Thank you for using this tool and taking the time to improve it!

I've added a couple of comments, and if you don't get time to take a look, I'll happily amend these when I do!

I'd like to reimplement the progress bar at some point (I started way back when, might've even had it working originally before switching to fetch!) but this is great as-is.

Let me know if you have any questions, but glad the contributions and DX has worked for you!

src/lib/Entry.ts Outdated Show resolved Hide resolved
translations/de.json Show resolved Hide resolved
src/lib/Entry.ts Outdated Show resolved Hide resolved
src/lib/handleFileUpload.ts Outdated Show resolved Hide resolved
@dom111 dom111 self-assigned this Dec 26, 2022
@Lex-2008
Copy link
Author

oh, missed your general comment previously. Yes, I agree that progress bar would be better, and I also was thinking about "Cancel" button to cancel each upload (since XHR can be cancelled), but let's keep these in TODO for now, since progress bar might need a discussion regarding size/location/color :)

@Lex-2008
Copy link
Author

Lex-2008 commented Jan 8, 2023

PR updated! I think all suggestions are addressed, please let me know if I missed some (I actually don't know what these letters S:, R:, T: stand for).

Also, let me know if there are any other improvements I can make here, or shall I cleanup the history for merge.

@Lex-2008 Lex-2008 requested a review from dom111 January 8, 2023 17:17
@dom111
Copy link
Owner

dom111 commented Jan 8, 2023

In general this looks great! I've added another comment above, shout if you have any concerns!

As for R:, S:, T:, I thought it was a general thing, but might be localised to where I've worked in the past, but: R: required change, S: suggested alternative, T: talk/discussion around the item.

Thanks for your patience on this!

dom111 and others added 6 commits January 17, 2023 22:30
Lets you track progress (pass a callback function as onProgress argument)
Also make arguments optional
Sadly, I don't speak these languages, so I used an online service
to translate "5 of 10 uploaded" sentense into them, and then updated
templates accordingly.
@Lex-2008 Lex-2008 force-pushed the xhr-upload-progress branch from 9af5156 to de92164 Compare January 17, 2023 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants