feat: allow passthrough of accepted image file types to display in image upload UI #304
Conversation
✅ Deploy Preview for stacks-editor ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Thanks for taking this on @kristinalustig!
We currently have a disconnect between the input and validation. Currently, we set the accept attribute on the input to image/* (so, any image type). When the user tries to upload something else, we show a validation message with jpeg, png, gif hardcoded:
In this PR, we don't address that but we are adding a field to pass file types. Right now, this is only used to generate caption text in the UI, but I think it can be used to populate the accept attribute on the input and ensure the validation message lines up with the file types we're accepting.
I've created #305 targeting this PR as a proposal for a slightly different approach to what's in this PR (I ran out of time on Friday so I haven't added a PR description yet). I think it matches the mocks/requirements and just needs a test or two added to image-upload.test.ts. Check it out when you get a chance and we'll see if this is a worthwhile approach. I'm happy to chat or pair with you on Monday to go over any details if you'd like.
* Refactor `acceptedFileTypes` to affect validation * Add function to generate the caption element * Minor tweaks so I can run tests * Add default accepted file types value
There was a problem hiding this comment.
I made some updates to how we handle the acceptedFileTypes option and how the they're displayed when rendered as text. I think this is in a good place to get merged.
Since I had a hand in much of these changes, I'd like to have @giamir give this a quick look just to make sure that I didn't do anything too funky and that the tests are sufficient.
There was a problem hiding this comment.
Overall looks good. Great job. 👏
I left a couple of small requests. One is challenging the product/design requirements to add an or (which is currently hardcoded). @kristinalustig you can explain to your designer, PM that the juice is likely not worth the squeeze for that or. Let us know if you need support in that conversation. 🙂
Tests are probably good enough I would say Dan. 👍
There was a problem hiding this comment.
@kristinalustig I removed the getAcceptedFileTypesString function since it was no only joining strings and removing image/ in two spots. All tests pass and this looks good to me 🎉
|
@kristinalustig we can merge this, do you want us to cut a new editor release with the changes straight away? @dancormier or I can do that instead of waiting till next week's "regular" release cycle. Let us know. Thank you for contributing to the editor codebase. 🎉 |
|
@giamir Ideally we can get it out sooner rather than later, but if that is a huge hassle it should be okay to wait until next week. |
No, either @dancormier or myself will cut a release today or tomorrow. No reason to wait. |
As discussed, this PR introduces a method by which users can pass through a list of strings representing different allowed media types. It then constructs an appropriate string listing out those media types and puts them into the image uploader dialog. Note that I am not sure what types of tests would be appropriate for this sort of change, if any, so if you think this change is large enough to require one, please let me know and I will update.
PR Checklist
/** ... */docsbug/enhancementand other labels as appropriateAdditional context
For reference, this pairs with SO codebase changes PR # 19347 - this one has to be in first so the build doesn't fail for SO.