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

Basic image uploader #2239

Merged
merged 2 commits into from Oct 25, 2021
Merged

Basic image uploader #2239

merged 2 commits into from Oct 25, 2021

Conversation

Aierie
Copy link
Contributor

@Aierie Aierie commented Oct 18, 2021

Basic image uploader + validation of image.

Image editing + upload to storage to follow in separate PRs, may use cropper.js for image editing as suggested by @jurgenwerk.

To view the component easily, see /image-uploader-temp.

@Aierie Aierie force-pushed the cs-2173-basic-image-uploader branch 4 times, most recently from 8d23c79 to 2597046 Compare October 21, 2021 07:25
@Aierie Aierie changed the title Cs 2173 basic image uploader Basic image uploader Oct 21, 2021
@Aierie Aierie force-pushed the cs-2173-basic-image-uploader branch 2 times, most recently from a3967b2 to 3f406a4 Compare October 21, 2021 12:06
@Aierie Aierie marked this pull request as ready for review October 21, 2021 12:20
@Aierie Aierie added the web web-client label Oct 21, 2021
@Aierie Aierie self-assigned this Oct 21, 2021
@Aierie
Copy link
Contributor Author

Aierie commented Oct 21, 2021

I think loading and disabled states might be necessary, do you think they are @jurgenwerk?

uploader!: HTMLInputElement;

@action setUploader(element: HTMLInputElement) {
this.uploader = element;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern is a hint that you could implement a modifier instead of a component to do this more easily.

See https://github.com/ember-modifier/ember-modifier

The did-insert modifier and friends are really more of a stepping stone to help people port classic Ember components to Glimmer components. Writing your own modifier is more expressive. They're especially good for integrating third-party DOM-manipulation libraries.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(It is unfortunately a little confusing that the ember-modifier addon doesn't ship in the default blueprint. IMO it should, and probably will eventually.)

Copy link
Contributor Author

@Aierie Aierie Oct 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will try and refactor this to a {{file-uploader}} modifier. Thanks @ef4!

Copy link
Contributor

@jurgenwerk jurgenwerk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

One comment.

I think it would be good to relax the image dimensions and size requirements to allow selecting photos in common dimensions. It's a big friction and focus breaker when users need to go and resize their images before uploading.

Ideally we would take the user's image and resize it ourselves to fit our limits. Can this be easily done in the browser before we send it to the backend?

@Aierie
Copy link
Contributor Author

Aierie commented Oct 25, 2021

Ideally we would take the user's image and resize it ourselves to fit our limits. Can this be easily done in the browser before we send it to the backend?

We could, but I think we would need an upsampling service in order to make enlarged/zoomed images look sharp. Users can make use of those themselves to resize and upsample if they are set on using smaller images.

I think setting minimum dimensions might be good in the meantime to guide the user to use better images. If we want to be lenient, failing the dimensions check could be ok, and just result in a warning that the image chosen is too small (or maybe nothing at all/we remove size validation - perhaps just a recommendation in text, rather than a requirement that is enforced is better?)

@jurgenwerk
Copy link
Contributor

Actually my thinking was the other way around–users might wanna choose images larger than 200KB and I was thinking it would be a good idea to support that (and downsize to fit our limits).

@Aierie Aierie force-pushed the cs-2173-basic-image-uploader branch from 3f406a4 to 3b0d38c Compare October 25, 2021 14:50
@Aierie Aierie merged commit 16bacb0 into main Oct 25, 2021
@delete-merged-branch delete-merged-branch bot deleted the cs-2173-basic-image-uploader branch October 25, 2021 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
web web-client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants