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

Uploader widget #1129

Open
tomdye opened this issue Mar 5, 2020 · 8 comments · May be fixed by #1540 or #1749
Open

Uploader widget #1129

tomdye opened this issue Mar 5, 2020 · 8 comments · May be fixed by #1540 or #1749

Comments

@tomdye
Copy link
Member

tomdye commented Mar 5, 2020

Please add all comments and discussions surround an uploader widget to this issue

@mwistrand mwistrand self-assigned this Mar 18, 2020
@mwistrand
Copy link
Contributor

mwistrand commented Mar 20, 2020

The native <input type="file"> is fairly limited in its behavior. For example, each time the user selects files, newly selected files are not added to an existing list but rather overwrite the existing file list. As such, while any uploader widget would use <input type="file"> under the hood, to be useful it would need to maintain its own internal File[] list that allows files to be added and removed in response to user actions.

At its most basic, <Uploader> would render a button that allows users to select files to upload, a list of selected files, as well as "delete" buttons for each file to remove them from the list. This basic implementation would accept a list of properties that are common to other form controls, as well as a few specific to <input type="file">:

interface UploaderProperties {
  // generic form properties
  disabled?: boolean;
  label?: DNode;
  name?: string;
  required?: boolean;
  onChange?: (selectedFiles: File[]): void;
  
  // specific to file uploading
  /**
   * File-type extensions or MIME types
   */
  accept?: string | string[];
  
  /**
   * Can multiple files be uploaded? Defaults to true.
   */
  multiple?: boolean;
}

A child renderer function could be provided to control the list of selected files. To expose "remove file" functionality to the child renderer, we have two options: add a files property to <Uploader>, or pass a remove function to the child renderer. Since security concerns prohibit browsers from specifying initial files for upload, adding a files property seems misleading and therefore I'm inclined to prefer a remove function:

<Uploader>
  {(files: File[], remove: (files: File | File[]) => void) => (
    <ul>
      {files.map((file) => (
        <li key={file.name}>
          {file.name}
          
          <button onclick={() => remove(file)}>
            {messages.removeFile}
          </button>
        </li>
      )}
    </ul>
  )}

In the future, it would be possible to add a drag-and-drop area (dnd={true}) and remove callbacks to allow custom functionality like upload-on-select:

<Uploader
  multiple={true}
  onChange={(files: File[]) => showProgressAndUpload(files.filter(notUploaded)}
  onRemove={(files: File[]) => showLoaderAndRemove(files)}
/>

@tomdye tomdye added medium Medium size issue new widget labels Mar 20, 2020
@msssk msssk self-assigned this Aug 25, 2020
@msssk
Copy link
Contributor

msssk commented Aug 26, 2020

UI mock for discussion:
https://codesandbox.io/s/admiring-forest-nx0sg?file=/src/main.tsx

The mock has 4 file rows:

  1. Upload in progress (only relevant if we provide XHR uploading)
  2. Upload error (only relevant if we provide XHR uploading)
  3. File selected for upload
  4. File selected for upload
  • How do we handle the <input type="file"> element? I'm assuming we want to hide it and use DOM that we can style better. If we do so we need some way of triggering click on it:
    • Call domNode.click() - don't think this is the Dojo way?
    • Wrap the element in a <label> - any problems/objections to this approach?
  • Do we want to pass files to a child renderer, or just render them ourselves and limit customization to whatever can be done with CSS?
    • I think if we go with a child renderer we should at least provide a default renderer so the widget is closer to being full-featured and just working out of the box
  • Do we want to provide any sort of upload process functionality or integration? e.g. accept an endpoint as a parameter and XHR/fetch to the endpoint?
    • If so we probably want to provide a non-immediate option
interface UploaderChildren {
    buttonLabel?: string; // Choose files…
    dndLabel?: string; // Or drop files here
    renderFiles?(props: { files: File[] }): RenderResult;
}

interface UploaderProperties {
    // generic form properties
    disabled?: boolean;
    name?: string;
    required?: boolean;
    onValue?: (selectedFiles: File[]): void;
  
    // specific to file uploading
    /**
     * File-type extensions or MIME types
     */
    accept?: string | string[];
  
    allowDnd?: boolean;

    /**
     * Can multiple files be uploaded? Defaults to true.
     */
    multiple?: boolean;
}

@tomdye
Copy link
Member Author

tomdye commented Aug 26, 2020

@msssk good questions / points so far. I think that we may well want multiple file uploaders to support different scenarios.
For example FileUploader / MultiFileUploader. We also need to ensure that these widgets work in a reactive manner and are capable of being used both controlled / partially controlled.

FileUploader

The basic uploader would use a label as you suggested to wrap the underlying input element and provide both a dojo and a material themed appearance.
The basic file uploader would have an onValue callback as you have specified which returns the uploaded file and would offer up the dnd / required / name etc properties you specified. It would not however handle multiple files and would accept only a label child. The dndLabel would most likely be a basic i18n message overridable via the i18nprocess.
I believe the uploader may also benefit from validation around filesize / type etc.

MultiFileUploader

The multi file uploader could well have an appearance similar to that of your code pen example but would be opinionated about how it renders it's children rather than offering a child renderer for them. It would use the FileUploader widget to upload the actual files and would allow both controlled and partially controlled interfaces for handling the uploaded files.
ie.

interface MultiFileUploaderProperties {
   value?: File[];
   initialValue?: File[];
   onValue(files: File[]) => void;
   onAdd(file: File) => void;
   onRemove(file: File) => void;
}

When the value property is set, the parent would be responsible for managing the value and adding / removing File entries, wheras if only the initialValue property was set, the multi file uploader would handle the adding / removing of files itself and would simply call back to the onValue property and inform the parent of the current file array.

The multi file uploader would also have the same kind of properties as the single file uploader such as required / dnd etc. It would probably also benefit from some validation such as max / min number of files?

This is far from extensive, but just my current thoughts on the above and the codepen example.

What do you think?

@tomdye
Copy link
Member Author

tomdye commented Aug 26, 2020

edit to the above, the multi probably only needs onValue and not onAdd / onRemove

@msssk msssk linked a pull request Aug 27, 2020 that will close this issue
7 tasks
@msssk
Copy link
Contributor

msssk commented Aug 27, 2020

I've created a draft PR (#1540) for this with a basic implementation and a few examples - Basic, Disabled, Multiple.

What would be the value of two separate widgets for single and multi file handling? One challenge I am seeing with that is the need to specify the multiple attribute on the native <input> element. If we don't specify that attribute then we remove a significant part of long-existing UX that allows the user to select multiple files at once from the OS file dialog. If we have a separate MultiFileUploader widget but FileUploader does not allow setting multiple, then we can't use FileUploader internally and basically have to reimplement all of FileUploader within MultiFileUploader.

  • The "Choose files" button is currently implemented as a <label> element wrapping the hidden <input type="file"> element. The label is styled as a button - this seems less than ideal, but I don't know what else to do unless we want to add middleware that allows you to invoke methods on a node so we can call fileInputNode.click()
  • I'm not sure of the best way to pass info to the child renderer - I do think it would be nice if we could pass the theme and i18n messages to the child renderer so it can use them

I think the next best things to work on are validation and DnD. After that maybe look into the widget itself handling XHR uploads? That would allow us to provide a renderer that handles progress display and errors.

@tomdye
Copy link
Member Author

tomdye commented Aug 27, 2020

The idea between the two different widgets would be to provide both a basic and a more complete widget with a file list etc. You could certainly still specify multiple as a property on the basic file uploader to achieve this. The more advanced one would provide the file list / add / remove file functionality as you proposed in your example.

  • re. the styling - looking at the Ant design uploader (https://ant.design/components/upload/) a button seems appropriate. We should probably be able to compose the Button styles across from button.m.css in order to keep this consistent.
  • re. child renderer, I'm not sure what we need to render as children aside from a label as per other input widgets.

@msssk
Copy link
Contributor

msssk commented Aug 28, 2020

I've split the widgets into two - a base FileUploadInput that is stateless and simply notifies its controlling widget of any file uploads, and a more full-featured FileUploader that controls a FileUploadInput and displays a list of added files.

File uploads and DnD are necessitating some DOM interactions not currently supported by Dojo:

  • In order to provide keyboard a11y we need to be able to call domNode.click() on the hidden input element. Is it ok to use vdom.node directly in FileUploadInput or should this be moved into middleware?
  • File DnD requires using DragEvent. @dojo/framework/core/middleware/drag provides more widely compatible DnD using pointer events, but this does not provide the DataTransfer API for handling files. I've created dnd middleware in the widgets repo
    • Should this be in widgets or framework?
    • How should the event listeners be removed?
    • Feedback on the API and code would be welcome

@tomdye
Copy link
Member Author

tomdye commented Mar 22, 2021

We should really get this over the line

@tomdye tomdye assigned tomdye and unassigned msssk Mar 22, 2021
@tomdye tomdye removed their assignment Mar 30, 2021
@bitpshr bitpshr linked a pull request May 7, 2021 that will close this issue
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants