feat(image-upload): add acceptedFileTypes option
#305
Conversation
dancormier
changed the base branch from
klustig/dynamic-accepted-file-types
to
main
dancormier
changed the base branch from
main
to
klustig/dynamic-accepted-file-types
| @@ -141,12 +144,9 @@ export class ImageUploader extends PluginInterfaceView< | |||
| this.uploadField = document.createElement("input"); | |||
| this.uploadField.type = "file"; | |||
| this.uploadField.className = "js-image-uploader-input v-visible-sr"; | |||
| this.uploadField.accept = "image/*"; | |||
| this.uploadField.accept = acceptedFileTypes?.join(", "); | |||
There was a problem hiding this comment.
does this make it so that it no longer reads with an "or" in the penultimate position?
There was a problem hiding this comment.
This property populates the accept attribute on the input itself by taking the array provided from the acceptedFileTypes option and joining values to create a string.
With the default acceptedFileTypes value, we get this:
<input … accept="image/jpeg, image/png, image/gif">| <div class="flex--item">${acceptedFileTypeString}</div> | ||
| <div class="flex--item">(Max size 2 MiB)</div> | ||
| </div> | ||
| </label>, drag & drop<span class="js-external-url-trigger-container d-none">, <button type="button" class="s-btn s-btn__link js-external-url-trigger">enter a link</button></span>, or paste an image. |
There was a problem hiding this comment.
Are these functionally different? They look the same to me when the string is displayed or not. Your correction seems like it's too complex for what we're trying to accomplish, but I may be missing something.
There was a problem hiding this comment.
This is purely a whitespace change. There was an extra space character at the end of the line that isn't necessary. I'm not sure why GitHub isn't showing it that way, but this is the diff for the change of line 158 (shortened for clarity):
- </label>[…] or paste an image.
+ </label>[…] or paste an image.There was a problem hiding this comment.
lol thanks github!
| return acceptedTypesString; | ||
| uploadCaptionString = acceptedTypes | ||
| .join(", ") | ||
| .replace(/image\//g, ""); |
There was a problem hiding this comment.
Why even have the image bit if we're just removing it any time we're displaying it? In this context it will always be an image.
There was a problem hiding this comment.
We're removing image/ when we're displaying it, but not always when we're using it. I figured it made the most sense to stick to the MIME type to imply the connection to the file input's accept attribute. I'd be fine with inverting this if you think the API would be simpler to instead have the consumer provide a list of file extensions.
Let me know what you think and I'd be happy to make changes if you'd like.
There was a problem hiding this comment.
I figured it made the most sense to stick to the MIME type to imply the connection to the file input's accept attribute.
FWIW sticking to the MIME type is a good call in my opinion. It will be easier for us to convey to the consumers how this option field should be populated - Furthermore people familiar with the HTML spec of input type="file" will have pretty much a zero learning curve. 🙂
dancormier
changed the title
acceptedFileTypes option
kristinalustig
merged commit 5cdca73
into
klustig/dynamic-accepted-file-types
This PR targets #304 and updates the image upload code to use the
acceptedFileTypesoption to:acceptattributeTODO