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

feat: added support for uppy #842

Merged
merged 24 commits into from
Jul 11, 2024
Merged

feat: added support for uppy #842

merged 24 commits into from
Jul 11, 2024

Conversation

ezhil56x
Copy link
Contributor

Added support for uppy with an example

Closes #807
/claim #807

Copy link

vercel bot commented Jun 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
blocknote ✅ Ready (Inspect) Visit Preview Jul 11, 2024 9:00am
blocknote-website ✅ Ready (Inspect) Visit Preview Jul 11, 2024 9:00am

Copy link

vercel bot commented Jun 13, 2024

@ezhil56x is attempting to deploy a commit to the TypeCell Team on Vercel.

A member of the Team first needs to authorize it.

@ezhil56x
Copy link
Contributor Author

@YousefED

Click to expand
2024-06-13.16-02-35.mp4

Copy link
Collaborator

@YousefED YousefED left a comment

Choose a reason for hiding this comment

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

This looks great already. I've left some comments inline but also have one main architectural request:

I don't think we want to add the uppy dependencies by default - otherwise users who don't need uppy will end up adding a lot of dependencies. This means all Uppy related code should live in the Example (or in a separate package (blocknote-uppy or sth), but I don't think that's necessary for now). The way to do this would be to in the example use a FilePanelController and pass in the Uppy tab as part of it's props. (the uppy tab should then also live in the configuration).

Secondly, I don't think we then need uploadFileUppy anymore in the configuration. We do need (see original requirements) to make sure that uploadFile uploads via Uppy. The reason for this is that while at this moment, it's true that uploadFile is only called from the upload tab (which we'll replace by the dashboard), but at a later point it's likely we'll call uploadFile from other places (like dropping files into the editor) as well.

@rajesh-jonnalagadda
Copy link

This looks great already. I've left some comments inline but also have one main architectural request:

I don't think we want to add the uppy dependencies by default - otherwise users who don't need uppy will end up adding a lot of dependencies. This means all Uppy related code should live in the Example (or in a separate package (blocknote-uppy or sth), but I don't think that's necessary for now). The way to do this would be to in the example use a FilePanelController and pass in the Uppy tab as part of it's props. (the uppy tab should then also live in the configuration).

Secondly, I don't think we then need uploadFileUppy anymore in the configuration. We do need (see original requirements) to make sure that uploadFile uploads via Uppy. The reason for this is that while at this moment, it's true that uploadFile is only called from the upload tab (which we'll replace by the dashboard), but at a later point it's likely we'll call uploadFile from other places (like dropping files into the editor) as well.

@ezhil56x I have already made changes related to this. If needed any reference you can view my closed pr.
#843

@ezhil56x
Copy link
Contributor Author

Will update my PR soon and let you know

examples/01-basic/11-file-uploading-uppy/App.tsx Outdated Show resolved Hide resolved
examples/01-basic/11-file-uploading-uppy/App.tsx Outdated Show resolved Hide resolved
examples/01-basic/11-file-uploading-uppy/App.tsx Outdated Show resolved Hide resolved
examples/01-basic/11-file-uploading-uppy/App.tsx Outdated Show resolved Hide resolved
@YousefED
Copy link
Collaborator

@YousefED If we need uploadFile then we should create a similar uploadFileUppy for handing uppy uploads. Will that be fine?

I think we just need something like this?

async function uploadFileViaUppy(file: File) {
uppy.addFile(file);
await uppy.upload();
}

...
uploadfile: uploadFileViaUppy

pseudo code, but I hope this clarifies the intended behavior for you

I tried this though, uppy.addFile() needs the file as blob type rather than file type. Creating a new uploadFileUppy like uploadFile will be a good option, but uppy packages should be added for that which increases the size. I am confused about what to do. What do you suggest?

see https://stackoverflow.com/a/33855825

@YousefED
Copy link
Collaborator

YousefED commented Jul 9, 2024

@ezhil56x I've updated the PR. As you can see I've been able to use:

  • both uploadFile and Dashboard
  • make sure the dashboard updates the correct block

I've also added some documentation, cleaned up the code a bit, and added some Uppy plugins so that it's a nice demo. Let me know what you think!

@ezhil56x
Copy link
Contributor Author

ezhil56x commented Jul 9, 2024

@YousefED
Great!
Tested, seems to be working good as expected

@matthewlipski matthewlipski merged commit 0e69fd4 into TypeCellOS:main Jul 11, 2024
4 checks passed
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.

Add support for Uppy for file uploading
4 participants