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

Added uppy for file uploading #840

Closed
wants to merge 3 commits into from
Closed

Added uppy for file uploading #840

wants to merge 3 commits into from

Conversation

harshithpabbati
Copy link

Fixes #212

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/is-reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@harshithpabbati
Copy link
Author

@jywarren @publiclab/is-reviewers done with it .

@harshithpabbati
Copy link
Author

it was same as the old one i just sent a new pr with rebasing.

@harshkhandeparkar
Copy link
Member

Harshith please don't open new PRs each time. It becomes really difficult to manage them. Thanks.

@harshithpabbati
Copy link
Author

Ok fine!!

@harshithpabbati
Copy link
Author

harshithpabbati commented Mar 6, 2019

@jywarren it's done i was also done with the rebase.

@jywarren
Copy link
Member

jywarren commented Mar 6, 2019

Oh, can you link to the previous PR so we can see what state things were at? Is this now ready for final review and can you attach a screenshot? Or could you try closing this and pushing the latest to the old PR so we stay in one thread?

Thank you @harshithpabbati !!!

@harshithpabbati
Copy link
Author

uppyup
This was the one and let's stay in this thread.

@jywarren
Copy link
Member

jywarren commented Mar 6, 2019 via email

@harshithpabbati
Copy link
Author

harshithpabbati commented Mar 6, 2019

Oh, is the Take a Photo button from within Uppy, or is it our own? I think Uppy has a feature like this that we could potentially use to replace our own?

Yeah it has a option but i was not able to do that. So i didn't removed the previous one.

@jywarren
Copy link
Member

jywarren commented Mar 6, 2019 via email

@harshithpabbati
Copy link
Author

harshithpabbati commented Mar 6, 2019

yeah i tried it many times but i was not able to make it out.

@harshithpabbati
Copy link
Author

We can make it as a new issue to add the take photo button using uppy.

@jywarren
Copy link
Member

jywarren commented Mar 6, 2019

Hmm, what line needed to be changed? Can you leave it in, but comment it out? And then we can continue to work on that here? I think it's best to try to do this as a single PR if possible, but if we get really stuck (even with help from others) we can move on and do it later. Thank you!

@harshithpabbati
Copy link
Author

We need to add the new lines in the script to use the web cam.

uppy.use(Webcam, {
  onBeforeSnapshot: () => Promise.resolve(),
  countdown: false,
  modes: [
    'video-audio',
    'video-only',
    'audio-only',
    'picture'
  ],
  mirror: true,
  facingMode: 'user',
  locale: {}
})

Something similar to this. I couldn't make I tried it for many times.

@jywarren
Copy link
Member

jywarren commented Mar 6, 2019

And what exactly went wrong, did you see an error?

examples/index.html Outdated Show resolved Hide resolved
examples/index.html Outdated Show resolved Hide resolved
@harshithpabbati
Copy link
Author

Yeah now i am done with the required changes.

@harshithpabbati
Copy link
Author

@jywarren check it out.

@harshithpabbati
Copy link
Author

@jywarren I tried it again today but i am unable to do it.
There was no output.

@jywarren
Copy link
Member

jywarren commented Mar 8, 2019

No output like nothing shows in the webcam video, or nothing is sent to the next step? Can you detail what happens with a few screenshots to illustrate? Are there errors shown when you try, like perhaps a permissions error?

Also, are the other means of uploading working fine?

Could we add upload by link?

image

@jywarren
Copy link
Member

jywarren commented Mar 8, 2019

No worries, we will figure this out! See if someone else from @publiclab/is-reviewers can also help debug a bit?

@jywarren
Copy link
Member

jywarren commented Mar 8, 2019

Could be that webcam access from a local file is security restricted? Can you try running a local server with the http-server npm module maybe? https://www.npmjs.com/package/http-server

@harshithpabbati
Copy link
Author

I will check through it.

@harshithpabbati
Copy link
Author

harshithpabbati commented Mar 8, 2019

No output like nothing shows in the webcam video, or nothing is sent to the next step? Can you detail what happens with a few screenshots to illustrate? Are there errors shown when you try, like perhaps a permissions error?

Also, are the other means of uploading working fine?

Could we add upload by link?

I was not getting anything.

@harshithpabbati
Copy link
Author

Ha did it but have changes in it @harshkhandeparkar
https://harshithpabbati.github.io/image-sequencer/examples/
Checkout this!!!

@harshkhandeparkar
Copy link
Member

Harshithz you have to force commit node_modules

@harshithpabbati
Copy link
Author

Harshithz you have to force commit node_modules

yeah i did it.

@harshithpabbati
Copy link
Author

but it was not working.

@harshithpabbati
Copy link
Author

it's not taking the images as input.

@harshkhandeparkar
Copy link
Member

Check if it is showing any errors in the browser

@harshithpabbati
Copy link
Author

Check if it is showing any errors in the browser
No it's working but i can't get the image into the load image

@harshkhandeparkar
Copy link
Member

Is it showing any errors in the browser console?

@harshithpabbati
Copy link
Author

harshithpabbati commented Mar 9, 2019

Yeah failing to load css files.

@harshkhandeparkar
Copy link
Member

Screenshot?

@harshkhandeparkar
Copy link
Member

You can comment or delete the part in index.html which links to the manifest.json file.

@harshithpabbati
Copy link
Author

console

@harshithpabbati
Copy link
Author

In localhost it is fine.

@harshkhandeparkar
Copy link
Member

Oh ok wait

@harshkhandeparkar
Copy link
Member

I checked out your branch, there is no node_modules

@harshithpabbati
Copy link
Author

oh ok fine but the problem is with getting the image in the load image....

@harshkhandeparkar
Copy link
Member

harshkhandeparkar commented Mar 9, 2019 via email

@harshithpabbati
Copy link
Author

It is not happening since jquery is not working as it is not available

On Sat 9 Mar, 2019, 9:51 PM Harshith pabbati, @.***> wrote: oh ok fine but the problem is with getting the image in the load image.... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#840 (comment)>, or mute the thread https://github.com/notifications/unsubscribe-auth/AhKOn_OkGA3tha4sNABX9V-KPdeoakEVks5vU999gaJpZM4bgYOZ .

even it's not happening in localhost

@harshkhandeparkar
Copy link
Member

If it is not happening on localhost, see if there are any errors in the console, again.

@harshithpabbati
Copy link
Author

local
see this.

@harshkhandeparkar
Copy link
Member

CORS problem 😭 please search for it on Google. I am busy I cannot help you right now sorry.

@harshkhandeparkar
Copy link
Member

Harshith, you will have to add an option to override the methods in this file https://github.com/publiclab/image-sequencer/blob/main/src/ui/SetInputStep.js. Instead of default methods for dropzone and fileInput, you will have to create an option to upload the file directly to sequencer, instead of sequencer handling the file upload.

@jywarren
Copy link
Member

Just a few notes -- i think CORS might stop being an issue if you use http-server to run your app locally. did you try this?

I like your solution of a popup. But perhaps it should say "Drag in an image or click here to upload" -- but then, Uppy wouldn't be running in the drag/drop zone itself. So, i think @harshkhandeparkar is right that you may have to refactor your work into SetInputStep. Does that make sense?

@jywarren
Copy link
Member

OH, wait - your CORS errors are on trying to upload using tus but i think you can just leave that out, as we don't need to upload -- just pass locally over to the Sequencer! See how SetInputStep accepts an options.onLoad method which it runs and passes in the image to:

var onLoad = options.onLoad;
var onTakePhoto = options.onTakePhoto;
var reader = new FileReader();
function handleFile(e) {
e.preventDefault();
e.stopPropagation(); // stops the browser from redirecting.
if (e.target && e.target.files) var file = e.target.files[0];
else var file = e.dataTransfer.files[0];
if(!file) return;
var reader = new FileReader();
reader.onload = onLoad;

@harshithpabbati
Copy link
Author

@jywarren I will resume my work I forgot about it.

@harshkhandeparkar
Copy link
Member

This is not working. We'll have to find a new solution. Since we are already developing a new UI, we will do it there. @harshithpabbati

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.

explore standard multi-source upload with Uppy package
3 participants