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

Add File Dropzone component #813

Draft
wants to merge 22 commits into
base: next
Choose a base branch
from

Conversation

mkitzmann
Copy link

This PR adds a File Dropzone as discussed in #118.

image

It consists of 3 seperate components:

  • DropHandler
  • FileItem
  • FileDropzone

The DropHandler is a utility component that simply enables drag-and-drop functionality for whatever is added inside its default slot.

The FileItem can be used to display a File including its filename and optionally its size. When the size attribute is defined it uses the FormatBytes component internally to display the bytes in a human-readable way.

The FileDropzone component provides an area for files to be dropped as well as a button to open the file-select dialog. When a url attribute is specified the selected file will be uploaded to the given url. The FileDropzone works for multiple Files by setting the max-files attribute. Each selected file is displayed below the dropzone using the FileItem component. Warnings and transfer errors will be shown separately for each file.

We have decided to not build a FileItemList component, since it would not have enough functionality to create a separate component.

We have created documentation and tests for each component.

We are looking forward to any feedback!

@vercel
Copy link

vercel bot commented Jul 4, 2022

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

Name Status Preview Updated
shoelace ✅ Ready (Inspect) Visit Preview Aug 22, 2022 at 9:58AM (UTC)

@claviska
Copy link
Member

claviska commented Jul 8, 2022

Thank you for submitting this! I'm still trying to carve out time to give it a proper review. Please bear with me!

@claviska
Copy link
Member

Thanks for submitting this!

For consistency, clarity, and familiarity, can we rename <sl-file-dropzone> to <sl-file-upload> and <sl-file-item> to <sl-file-upload-item>? There are two patterns for grouping components that Shoelace currently uses. One is the PARENT/PARENT-ITEM convention (e.g. menu + menu item) and the other is ITEM-GROUP/ITEM (e.g. button group + button). The current name follows neither, and the latter convention doesn't quite seem appropriate.

Drop Handler

  • I like that drag and drop logic has been abstracted into a utility, but it's not clear from the docs how one would use this. What exactly is its purpose? How would I use it outside of the file dropzone component?
  • What's the purpose of the dragged attribute? It seems like it mimics the same styles as when you drag a file over the region, but when would one use it? I also don't see it in the attributes/properties table.
  • I don't believe any component uses the isVerb convention for public properties, so can we rename isDragged to dragged to match the attribute?
  • What does dropEffect do exactly? There's a description, but no default value and DropEffect is the only thing listed as a type. (This is a limitation of the analyzer/docs generator + enums so adding possible values to the description is one easy way around it. I tend to avoid enums because of this, so another way is to use a union, e.g. 'copy' | 'move' | 'link' | 'none' even if that means the type is less reusable)
    • Looking at the code and docs, it's not clear what copy | move | link | none will do.
  • dropEffect should have a corresponding attribute if it's intended to be set in HTML.

I may have additional feedback on this one depending on your answers to the above questions :)

File Dropzone

  • I don't think the component should be responsible for interacting with the backend as described here. The component should replace <input type="file"> but the upload should be handled by the user. This makes the component less opinionated and easier to use, while allowing consumers to determine how/which lib they want to handle uploads.
    • No other components make such assumptions or submit data to a user-provided backend
    • An argument can be made that offloading this responsibility makes it harder to use the component. This is sorta true, but baking all this in makes the component less flexible. We can offset this difficulty by providing a few examples, e.g. "Uploading with a Form," "Uploading with window.fetch," "Uploading with Custom Headers," etc.
    • Ideally, users will be able to swap out <input type="file" multiple> with this component and things "just work"
  • Love the button-only example, although this demo makes the case for changing the name to <sl-file-upload>
  • Is it possible to customize the button's size/variant/shape/etc.? Perhaps the best way to do this is with a slot such as <slot name="button"> with default content.
    • We should probably make the standard button variant default
    • We'll also need to rethink how the entire thing turns "warning" when an error is shown. If buttons are slotted, we don't want to mess with them. We can play with some designs here.
  • I have mixed feeling about the label property + icon slot + content slot. In all other form controls, label is both a prop and a slot where the default content is ${this.label}.
    • We should probably keep this convention going, meaning content would become label.
    • The icon slot should probably be named image or something similar. This is aria-hidden content that serves as a visual cue to sighted users, so it merits its own slot separate from content/label. The example with the QR code in the icon slot seems to support this. (Would there ever be anything other than an icon/image in here?)
    • We probably shouldn't encourage interactive elements like the card with rating in the "Custom Content" example. If users want custom drop zones that aren't supported through the styling API, they should use Drop Handler to make their own.
  • Is there a use case for no-drag? Is there ever an advantage to turning it off? If not, we should probably remove this attribute.
  • Same with closable. I don't think we need this. If you select a file, you should be able to unselect it before submitting. Is there a use case I'm missing? Perhaps one that has to do with composition?
  • All <sl-icon> elements in shadow roots (i.e. icons that are not slotted by the user) need to use the system icon library and the associated icons need to be added here.
  • In this example, the icon is larger than the other examples. The only noticeable difference is this example has the size in bytes whereas the others don't. I don't think the icon size should be affected by the presence or absence of a size (this is something users could configure if they wanted to change it).
  • I appreciate the use of spacing tokens for consistency! Alas, the upload icon is centered in its viewbox making the gap look inconsistent. On one hand, I'm reluctant to fix the gap because alternate icons will have the wrong spacing, but on the other hand, this is the default icon and it looks off. Maybe we crop the SVG and inline it for this one instead of using <sl-icon>? This GIF shows the gap and the invisible icon boundary that causes it in red.
    CleanShot 2022-07-11 at 10 30 44

File Item

  • Can we have it show a thumbnail when an image is selected? Maybe a cover version in a fixed-size box where the icon would normally be.
  • The icon part should be named image for the same reason mentioned in File Dropzone
  • The overall design looks nice, but it's a bit large. Can we use --sl-spacing-medium instead of --sl-spacing-large and reduce the font size of the bytes, e.g.

CleanShot 2022-07-11 at 10 20 12@2x

Other

  • Once composition is figured out 100%, we should look at exporting "subparts" using the convention outlined here which is the same as the rest of the library. I'm happy to help with this!

Appreciation

OK, that's a lot! 😅 Great work thus far. This is a complex component, and I'm thrilled to see such a thorough PR. A few final points, since these things always come across as criticism/negative and that's definitely not the intent!

  • Thank you for contributing!
  • Thank you for following existing patterns and guidelines!
  • Thank you for being so comprehensive with your design and implementation!
  • Thank you for writing tests for these components!

@mkitzmann
Copy link
Author

mkitzmann commented Jul 28, 2022

Thank you @claviska for your detailed review!

can we rename <sl-file-dropzone> to <sl-file-upload> and <sl-file-item> to <sl-file-upload-item>

We can do that. Altough you mention that the component should not actually have any upload functionality, so alternatively they could be named <sl-file-input> and <sl-file-input-item>.

DropHandler

After some reconsideration we would suggest to remove the DropHandler component entirely and instead use native events in the FileDropzone. The current functionality does not seem to warrant a separate component.

FileDropzone

I don't think the component should be responsible for interacting with the backend

It sounds like a good idea to reduce complexity in this component. We can remove the upload functionality and create examples in the documentation.
The only issue with this is that it is not clear how to show the upload progress in the FileItems when the upload is not handled inside the component. Do you have an idea how this could be achieved?

Ideally, users will be able to swap out <input type="file" multiple> with this component and things "just work"

That would indeed be nice. In this case we would need to provide a way for the component to properly work in a form. Looking at the other form components and the documentation they seem to only instantiate a FormSubmitController and not actually do anything with it. Maybe you can share some of your experience how to get this component to work in a form.

Is it possible to customize the button's size/variant/shape/etc.? Perhaps the best way to do this is with a slot such as <slot name="button"> with default content.

Good point. We should then probably move the click listener further up so it triggers even when there is a different element in the slot.

We should probably make the standard button variant default

We will make the standard button variant default

We'll also need to rethink how the entire thing turns "warning" when an error is shown. If buttons are slotted, we don't want to mess with them. We can play with some designs here.

Can you clearify what you mean with this?

I have mixed feeling about the label property + icon slot + content slot. In all other form controls, label is both a prop and a slot where the default content is ${this.label}

We will rename content to label and icon to image.

We probably shouldn't encourage interactive elements like the card with rating in the "Custom Content" example. If users want custom drop zones that aren't supported through the styling API, they should use Drop Handler to make their own.

Good point. Further customization could also be achieved with native elements if we remove the DropHandler.

Is there a use case for no-drag? Is there ever an advantage to turning it off? If not, we should probably remove this attribute.
Same with closable. I don't think we need this. If you select a file, you should be able to unselect it before submitting. Is there a use case I'm missing? Perhaps one that has to do with composition?

We will remove no-drag and closable.

All <sl-icon> elements in shadow roots (i.e. icons that are not slotted by the user) need to use the system icon library and the associated icons need to be added here.

We will add the icons to the system icon library.

  • In this example, the icon is larger than the other examples. The only noticeable difference is this example has the size in bytes whereas the others don't. I don't think the icon size should be affected by the presence or absence of a size (this is something users could configure if they wanted to change it).

We will leave the icon size of the FileItem the same when size is set.

I appreciate the use of spacing tokens for consistency! Alas, the upload icon is centered in its viewbox making the gap look inconsistent. On one hand, I'm reluctant to fix the gap because alternate icons will have the wrong spacing, but on the other hand, this is the default icon and it looks off. Maybe we crop the SVG and inline it for this one instead of using <sl-icon>? This GIF shows the gap and the invisible icon boundary that causes it in red.

We will inline the bi-cloud-arrow-up svg in the dropzone.

FileItem

Can we have it show a thumbnail when an image is selected? Maybe a cover version in a fixed-size box where the icon would normally be.

We would like to add this in another PR so we can reduce the complexity. Would it be ok for you to add this as a Feature Request and get back to it later?

The icon part should be named image for the same reason mentioned in File Dropzone

We will rename icon to image.

The overall design looks nice, but it's a bit large. Can we use --sl-spacing-medium instead of --sl-spacing-large and reduce the font size of the bytes

We will adjust the spacing and font size.

Other

Once composition is figured out 100%, we should look at exporting "subparts" using the convention outlined here which is the same as the rest of the library. I'm happy to help with this!

Thank you very much. Your help would be appreciated.

We are looking forward to your response.

@claviska
Copy link
Member

After some reconsideration we would suggest to remove the DropHandler component entirely and instead use native events in the FileDropzone. The current functionality does not seem to warrant a separate component.

Sounds good!

Looking at the other form components and the documentation they seem to only instantiate a FormSubmitController and not actually do anything with it. Maybe you can share some of your experience how to get this component to work in a form.

This is my workaround for not having ElementInternals in all browsers yet. When the FormSubmit controller is linked to a component, a number of things happen when said component is connected to the DOM.

https://github.com/shoelace-style/shoelace/blob/next/src/internal/form.ts#L55-L85

The controller taps into the formdata event to append data before the form is submitted. Additionally, it hooks into the containing form for submit and reset events.

When the controller is instantiated, you can pass options to customize how name, value, defaultValue, disabled, etc. are mapped. The reason it looks barebones in most components is because, if the component follows native APIs, the default values will be used.

https://github.com/shoelace-style/shoelace/blob/next/src/internal/form.ts#L33-L53

Aside from instantiating and optionally mapping those properties, the FormSubmit controller handles the rest.

We'll also need to rethink how the entire thing turns "warning" when an error is shown. If buttons are slotted, we don't want to mess with them. We can play with some designs here.

Can you clearify what you mean with this?

When the warning state is shown, everything turns amber.

CleanShot 2022-07-28 at 10 03 42@2x

But if we allow the user to slot in buttons and such, we won't want to manipulate their buttons, meaning you could end up with something like this:

CleanShot 2022-07-28 at 10 05 08@2x

So we should probably revisit the error styles to account for this.

Can we have it show a thumbnail when an image is selected? Maybe a cover version in a fixed-size box where the icon would normally be.
We would like to add this in another PR so we can reduce the complexity. Would it be ok for you to add this as a Feature Request and get back to it later?

Absolutely! And thanks again for the PR!

@mkitzmann
Copy link
Author

I don't think the component should be responsible for interacting with the backend

It sounds like a good idea to reduce complexity in this component. We can remove the upload functionality and create examples in the documentation.
The only issue with this is that it is not clear how to show the upload progress in the FileItems when the upload is not handled inside the component. Do you have an idea how this could be achieved?

@claviska Do you have an idea how to to handle upload progress?

@claviska
Copy link
Member

claviska commented Aug 8, 2022

Do you have an idea how to to handle upload progress?

That would be provided by the XHR so the user will need to wire it up to their request. If they're using axios, for example:

axios.request({
    method: 'post', 
    url: '/upload', 
    data, 
    onUploadProgress: (p) => {
      const percentage = p.loaded / p.total;
      //
      // pass percentage back to the uploader here
      //
    }
}).then(...);

I'll admit it's less convenient than just providing a URL, but it allows the user to handle uploading with whatever utility they want and the component doesn't have to support all fetch/XHR options, including more complex features such as auth, resuming failed uploads, server-side validation conventions, handling various server-side error responses, chunking uploads, large file uploads, etc.

To make things easier, I would recommend writing an adapter for the component that does that wiring for your API or library of choice. It's not something I'd prefer to maintain myself, though.

@mkitzmann
Copy link
Author

mkitzmann commented Aug 10, 2022

@claviska We updated the discussed changes.

Looking forward to any feedback!

@preethivenkatesh-sdase
Copy link

@claviska Could you approve the deployment workflow for the new commits made by me, please?

Also, FYI, as @mkitzmann is on vacation for next couple of weeks, I would be part of the discussions till he is back! :)

Thank you in advance, and we are looking forward to your feedback on recent changes!!

@claviska
Copy link
Member

Approved, and apologies for the delay. I need to carve out some time to focus on this, which I promise to do soon!

@mkitzmann
Copy link
Author

@claviska Is there anything we can do to help with review process?

@claviska
Copy link
Member

claviska commented Sep 7, 2022

@claviska Is there anything we can do to help with review process?

This is 100% on me, and I sincerely apologize. I haven't had as much spare time as I'd like to tackle this, but I will be circling back to this PR soon! Thanks for your patience!

@petergrau
Copy link

petergrau commented Oct 11, 2022

@claviska Is there anything we can do to help with review process?

This is 100% on me, and I sincerely apologize. I haven't had as much spare time as I'd like to tackle this, but I will be circling back to this PR soon! Thanks for your patience!

@claviska were you able to look at our updates? (we are approaching some team changes over here and will probably have less time in the future for the design system ;(

@claviska
Copy link
Member

I'm really sorry. I had a big push at work a couple weeks ago, followed by a vacation during which I caught a nasty cold. 😭

I'm running behind on some things — this being one of them. I really appreciate the effort that's gone into this and want to give it a proper review as soon as I'm able. Thanks again for your patience!

@tpluscode
Copy link

Please accept an explicit 👍 as a token of interest in this functionality

@claviska
Copy link
Member

claviska commented Jan 3, 2023

Here's where I'm at with this PR — and I don't want to minimize anyone's contribution in any way — but I think since the scope was scaled down from the original submission, this can probably be implemented in a simpler way without the need for an additional file item component.

I've been wanting to experiment with this to make sure (and hopefully I'll be able to soon), but my thought is that file items can be made part of the template with parts that the user can customize with CSS. The template will essentially mirror this.files and be updated when the user selects them.

I can't foresee a need for users to declaratively populate a file input since they wouldn't have permission to access those files anyway without the user explicitly selecting them first.

@mkitzmann
Copy link
Author

Thank you @claviska for your feedback.
I am always in favor of a simpler approach, so I would be fine without a FileItem Component.
The reason we did this was to be more consistent with the Menu/MenuItem approach.

The other problem now is, that Im no longer part of SDA-SE and therefore dont have write access to that repo. I guess I could fork the repo and open another PR. I will contact my former colleagues about this just to be sure.

@preethivenkatesh-sdase
Copy link

Hi @claviska Thank you for your feedback.
As @mkitzmann already mentioned, we did the changes to be aligned with Menu/MenuItem approach. I am also in favor of the simpler approach.

Sadly, as @mkitzmann is no longer with SDA-SE, he cannot make changes to our repo anymore! I will update the PR in couple of days with the mentioned changes. @mkitzmann If you wish, please feel free to open up a PR to against the SDA-SE FileDropZone branch ;-) And also please do the review after my changes if possible. :)

@claviska
Copy link
Member

Thanks, all. I have a big announcement to make soon, so please hold off on any work here for a little while. 😄

@claviska
Copy link
Member

claviska commented Mar 3, 2023

Here's a proof of concept for a simpler implementation that uses just one component. It's already wired up and submitting correctly. As you can tell, I haven't spent much time on the design and it's missing drag & drop, error handling, parts, etc., but this should bring some more clarity to my vision and why I've held off on this PR.

If you view the preview, make sure to open the dev console when submitting to see the result.

View the preview on Vercel

View the source on GitHub

@mkitzmann
Copy link
Author

Here's a proof of concept for a simpler implementation that uses just one component. It's already wired up and submitting correctly. As you can tell, I haven't spent much time on the design and it's missing drag & drop, error handling, parts, etc., but this should bring some more clarity to my vision and why I've held off on this PR.

If you view the preview, make sure to open the dev console when submitting to see the result.

View the preview on Vercel

View the source on GitHub

Hey @claviska,
thank you very much for your feedback! I like the simpler approach and also naming it "file-input" instead of "file-upload".
@preethivenkatesh-sdase I would be more than happy to review your changes to the PR. If you wont be able to work on this any more I can also take over and open a PR against your fork.

@claviska
Copy link
Member

Let's hold off on this for now so I can plan things out with the team. We may decide to take this one in house out of necessity. Thanks all! Your efforts are much appreciated!

@claviska claviska mentioned this pull request May 1, 2023
@petergrau
Copy link

Let's hold off on this for now so I can plan things out with the team. We may decide to take this one in house out of necessity. Thanks all! Your efforts are much appreciated!

@claviska any update from your side?

@claviska
Copy link
Member

Thanks for your patience, and apologies for the delay. Shoelace now has a full-time team of three people, including a fantastic designer, and we're going to start rolling out some of these core components very soon!

@dagnelies
Copy link

dagnelies commented Sep 21, 2023

Any news? It would be really great to merge this long awaited feature! 👀
In the worst case, it can still be improved later on, right?

As for my 2 cents feedback, I'd also suggest to make the whole dropzone a button instead of having an extra button "browse files" inside. ;) Nonetheless, great job guys! 👍

Edit: sorry, I read the whole thread and I see the big picture now. Basically, the PR was made ages ago and forever delayed because of its complexity. I can uderstand but it's a pity though. This hard work is at risk of being lost. Regarding your simplified component suggestion, it looks oversimplified to be honest. The important functionality here is the dropzone after all.

@claviska
Copy link
Member

We have a plan for this one to be finished and released early next year, along with a lot of great improvements to theming. Lots of great stuff is happening. Thanks for your patience!

@claviska claviska marked this pull request as draft January 23, 2024 15:46
@a7medm7med
Copy link

@claviska any news? This component is very important.

@klaarseSICKAG
Copy link
Contributor

Any updates on the timeline for this component?

@yringler
Copy link
Contributor

yringler commented Apr 4, 2024

It seems that future additions to shoelace will only be available via payed subscription, and drop zone doesn't seem to be on the immediate to do list (source - see stretch goals)

It seems that if someone in the community wants to create an open source component, the way to do that would be as part of a separate project (as it seems @klaarseSICKAG is doing).

It would be convenient if there would be an official shoelace (or web awesome) community edition which would be quicker to accept community PRs, but

  1. There would have to be someone in the community with the means and interest of working on that
  2. font awesome would have to be up for hosting their competition, as it were.

@claviska
Copy link
Member

claviska commented Apr 4, 2024

It seems that future additions to shoelace will only be available via payed subscription, and drop zone doesn't seem to be on the immediate to do list

On the contrary, it’s not listed as a stretch goal because we’re planning on the file input to be part of free along with a number of other components. We plan on adding more to Web Awesome free just like Font Awesome free continues to add more icons. We may add a more complex uploader under pro, but we haven’t discussed specifics of that yet. (You may have noticed that most of the pro components are the more complex ones.)

It would be convenient if there would be an official shoelace (or web awesome) community edition which would be quicker to accept community PRs, but

There would have to be someone in the community with the means and interest of working on that

Remember that you can use any web component you want with Shoelace/Web Awesome. The reason we’re offering a pro version is so we can keep building this stuff for you.

Bear in mind that I had to take a bit of a break to assemble a team and get things in order. I appreciate your continued support and patience as we transition.

@yringler
Copy link
Contributor

yringler commented Apr 4, 2024

Gotcha.
Thank you very much for the clarification, and all of your hard work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants