REFACTOR: extracting uploads from chat-composer code (PR #511)

  • attempts to test chat-composer-uploads and chat-composer-upload in isolation, note that inProgressUploads are currently very hard to test
  • slightly simplify styling
  • adds a halo when drag & dropping files on the textarea
  • moves css code in dedicated files


Kapture 2022-01-13 at 14 51 00

I’ve mentioned this before but I dislike that we are using a class to send information between components. It should be some kind of service.

I checked and this property and any related code is no longer needed.

I can see what you mean about the complexity of inProgressUploads and the difficulty of testing it. This array is mutated on the uppy "upload" event and the "upload-success" event. In core I was able to test this by adding an add-files appEvent which passes the files directly into uppy. See:

Apart from this we have more of a problem with the chat uploads though. At first the ComposerUploadUppy mixin was used because the uploads in chat acted in a similar way to the uploads in composer (e.g. writing the markdown into the chat area for submission). However this has now changed – uploads for chat are a separate component and have nothing to do with the chat composer or markdown at all really. It’s more akin to the uppy-image-upload component in core.

So before further refactors here, I would like to use this PR of yours as a base to change the chat-composer-uploads components to use the UppyUploadMixin which is much simpler and doesn’t involve any markdown or weird composerModel things. It will have a lot less to override that way, and I think that way it will become cleaner for sure, then testing inProgressUploads will just be a matter of adding the add-files and other appEvents to UppyUploadMixin so tests can more easily interact with it.

LMK what you think…so far this PR is a big improvement, It does also make me wonder if having the ComposerUploadUppy mixin in core is even necessary, or if the composer uploads there can be changed to have a similar pattern. Will think about that more.

This seems weird…can’t we just change this to showUploadsContainer(uploadCount, inProgressUploadCount)?

I think I can make a core change to just ignore this property if it’s null, we don’t need it for chat, it’s only used if you need to mess with the upload markdown, and since we aren’t even using that markdown in chat we can just throw it away. I can make a companion core PR perhaps that we can use to clean up after this one.

Another possible candidate to drop for composer, it’s only used in core to stop duplicate markdown for the upload placeholders:

typo uplods → uploads

We have this exact same thing in core but we also check for acceptsAllFormats, which is in the JS code for chat uploads but never used in the template. Here is core:

Perhaps we could make a really tiny component for upload file input, that includes the acceptedFormats and acceptsAllFormats functions? {{upload-file-input id={{fileUploadElementId}} }}

It’s interesting we don’t have a “cancel all uploads” button in chat. IDK if we need it or not, just observing. In core this sets uploadCancelled on the composerModel:

This function is not used, see my comment in the .hbs template.

I can change core’s _findMatchingUploadHandler function to say if (!this.uploadHandlers) { return; } so we don’t have to have this stub override.

This is another thing used in core’s _uploadPlaceholder function that we don’t care about…I have some thoughts forming now, will type them up in the final review box.

Leaving this here but nevermind it, see the main PR comment.

Leaving this here but nevermind it, see the main PR comment.

Leaving this here but nevermind it, see the main PR comment.

Leaving this here but nevermind it, see the main PR comment.