-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
[BUU] Update product images #12031
[BUU] Update product images #12031
Conversation
a4bd2c5
to
dcaf68d
Compare
Hi @mariocarabotta , this can be previewed on staging. While working on this, I've generated some example errors by uploading a non-image file: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done.
= f.label :attachment, t(".upload"), class: "button primary icon-upload-alt" | ||
= f.file_field :attachment, style: "display: none" | ||
= f.submit # todo: submit automatically |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the future will be to upload the files directly to the storage (AWS) from the browser. Rails has everything we need for that. The form submission then just contains the id of the uploaded file and a checksum and should work with StimulusReflex. The downside of this approach is probably that the storage validations gem doesn't work with this. It inspects the uploaded files and we would need to download it from the storage first. Still possible. We need to download it anyway to generate all the variants.
@mkllnk , you pointed out that we can validate the image types on the client side. Sound ok to you? |
dcaf68d
to
9d8b7e5
Compare
Rebased to resolve conflict. |
Yes, perfect. |
is that actually one error with 2 different explanations? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good ! It just looks like there is a leftover method in the products_reflex
Yes, you've guessed correctly. I took a look and found that the second error has been added as a more user-friendly message. It looks like it was intended to replace the first one, but for some reason they both show. Edit: before we deal with error messages, I'd like to finish off the current approach, as that will dictate what's practical going forard. |
Awaiting dependency merged before this can proceed. |
sure sure no rush on dealing with errors. let's sort it out when we have the full picture of everything else |
It's more flexible. Also reduced the overall size to suit the desired table row sizes.
The style was already being shared with a sibling component. Now we can instantiate a plain old 'modal'.
Using pre-existing feature in ModalController
It submits to the existing controller. I wanted to submit it with StimulusReflex, but it [doesn't support file uploads](https://docs.stimulusreflex.com/guide/working-with-forms.html#a-note-about-file-uploads). Perhaps we'll enhance this with javascript later.
This could have been done with a tiny amount of inline JS, but I went this way in case I needed any special logic for UJS. It turns out we don't.. but it still looks a bit cleaner this way.
I think we need a more general solution similar to ConfirmModalComponent's .modal-actions so that we can style all modals more consistently. But it's too late in the afternoon.
Leaving the more specific validation in the Rails model (rather than try to duplicate it).
This required some branching in the haml, which would be nice to avoid. But we have more work to do here so we'll refactor at the end.
0e169ad
to
627863b
Compare
Rebased, and added an update for hover behaviour and style fix. |
What? Why?
This also includes some visual updates.
I'm taking baby steps with this one so I can get dev input on the approach:
What should we test?
As per #11065
But you can only update existing images, not add a new one yet.
Release notes
Because
admin_style_v3
has now been released to instance managers:The title of the pull request will be included in the release notes.
Dependencies
Documentation updates