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

[BUU] Update product images #12031

Merged

Conversation

dacook
Copy link
Member

@dacook dacook commented Jan 11, 2024

What? Why?

I'm taking baby steps with this one so I can get dev input on the approach:

  1. The modal and form for the chosen image are loaded on-demand, with a Reflex
  2. It submits a standard form to the existing ImagesController, because Reflex doesn't support file uploads.
    • Perhaps we'll submit this with AJAX later, instead of reloading the whole page. But we'll probably focus on other parts of the page first.
  3. When updated, it redirects back to the product list (keeping any existing query params)

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:

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@dacook dacook self-assigned this Jan 11, 2024
@dacook dacook force-pushed the buu/update-product-images-11065 branch from a4bd2c5 to dcaf68d Compare January 11, 2024 04:39
@dacook
Copy link
Member Author

dacook commented Jan 11, 2024

Hi @mariocarabotta , this can be previewed on staging.

While working on this, I've generated some example errors by uploading a non-image file:

Screenshot 2024-01-11 at 3 26 45 pm

@dacook dacook marked this pull request as ready for review January 11, 2024 04:57
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Nicely done.

Comment on lines 13 to 15
= f.label :attachment, t(".upload"), class: "button primary icon-upload-alt"
= f.file_field :attachment, style: "display: none"
= f.submit # todo: submit automatically
Copy link
Member

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.

@dacook dacook added the feature toggled These pull requests' changes are invisible by default and are grouped in release notes label Jan 11, 2024
@dacook
Copy link
Member Author

dacook commented Jan 12, 2024

@mkllnk , you pointed out that we can validate the image types on the client side.
The browser can do this for us, so I suggest we add the simple mime-type condition image/*, and leave the more specific validation in the Rails model (rather than try to duplicate it).
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file#unique_file_type_specifiers

Sound ok to you?

@dacook dacook force-pushed the buu/update-product-images-11065 branch from dcaf68d to 9d8b7e5 Compare January 12, 2024 01:30
@dacook
Copy link
Member Author

dacook commented Jan 12, 2024

Rebased to resolve conflict.

@mkllnk
Copy link
Member

mkllnk commented Jan 12, 2024

The browser can do this for us, so I suggest we add the simple mime-type condition image/*, and leave the more specific validation in the Rails model (rather than try to duplicate it).

Yes, perfect.

@mariocarabotta
Copy link
Collaborator

Hi @mariocarabotta , this can be previewed on staging.

While working on this, I've generated some example errors by uploading a non-image file:

Screenshot 2024-01-11 at 3 26 45 pm

is that actually one error with 2 different explanations?

Copy link
Collaborator

@rioug rioug 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 ! It just looks like there is a leftover method in the products_reflex

app/reflexes/products_reflex.rb Outdated Show resolved Hide resolved
@dacook
Copy link
Member Author

dacook commented Jan 15, 2024

is that actually one error with 2 different explanations?

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.
We can try to fix it when we work on error messages. I'll add a suggestion for this on the issue.

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.

@dacook
Copy link
Member Author

dacook commented Jan 15, 2024

Awaiting dependency merged before this can proceed.

@mariocarabotta
Copy link
Collaborator

is that actually one error with 2 different explanations?

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. We can try to fix it when we work on error messages. I'll add a suggestion for this on the issue.

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.

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.
Refactor to use the (previously unused) shared method.
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.
@dacook dacook force-pushed the buu/update-product-images-11065 branch from 0e169ad to 627863b Compare January 16, 2024 02:45
@dacook
Copy link
Member Author

dacook commented Jan 16, 2024

Rebased, and added an update for hover behaviour and style fix.

@mariocarabotta mariocarabotta added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Jan 16, 2024
@dacook dacook merged commit dd502f0 into openfoodfoundation:master Jan 16, 2024
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature toggled These pull requests' changes are invisible by default and are grouped in release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants