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] See product images #11942

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

dacook
Copy link
Member

@dacook dacook commented Dec 15, 2023

What? Why?

Displays each product's image, and provides a shortcut to the edit page.
Sorry it should have been two separate commits, I hope it's ok to review as-is.

Screen Shot 2023-12-15 at 4 54 38 pm

What should we test?

Viewing images
Given that I am in /admin/products
And I have products with an image
When my catalogue has loaded
Then I see the illustration for products

Interim scenario: Updating an image
Given that I am hovering on a product
When I select to edit its image
Then I see the image edit page and can update it.

Note that products without images don't have any new options yet. This will be expanded upon in the next part.

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • 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

This comment was marked as outdated.

@dacook dacook self-assigned this Dec 15, 2023
@dacook dacook changed the title [BUU] Editing product images [BUU] See product images Jan 9, 2024
@dacook dacook marked this pull request as ready for review January 9, 2024 05:46
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.

Nice one !

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.

Nice.

@dacook dacook mentioned this pull request Jan 11, 2024
4 tasks
@RachL RachL added the pr-staged-fr staging.coopcircuits.fr label Jan 11, 2024
@RachL
Copy link
Contributor

RachL commented Jan 11, 2024

@dacook @mariocarabotta one question here: the button "edit" appears as soon as we hover the line. does this mean that we will be able to edit the image by clicking anywhere on the line? Currently the button works only when we click on it.

If in the end we keep only the click on the button, maybe we should always display it instead of displaying it on hover? Or can we display it only when hovering the image?

@RachL RachL added feedback-needed and removed pr-staged-fr staging.coopcircuits.fr labels Jan 11, 2024
@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

To explain the current behaviour: on hover, we're simply revealing what options we have (we also reveal the text inputs). You can then click to the action that you want.

But I can now see how the button does appear to be related to the whole row. An idea: what if instead of a button, the image changed another way, eg an icon overlaid, to indicate that it's clickable? Rough mockup:
Screenshot 2024-01-12 at 12 19 02 pm
Hmm actually I don't think that makes much difference.

What if the button/icon don't show until you hover the actual image. Maybe it's easy enough for the user to discover, so we don't need to show anything on row hover.

Hmm I think we're over-thinking it!

@dacook
Copy link
Member Author

dacook commented Jan 12, 2024

Rebased to resolve conflict.

@RachL
Copy link
Contributor

RachL commented Jan 12, 2024

What if the button/icon don't show until you hover the actual image. Maybe it's easy enough for the user to discover, so we don't need to show anything on row hover.

That's the behavior on the old page FYI. It's not perfect but maybe we can keep it for now?

@dacook
Copy link
Member Author

dacook commented Jan 15, 2024

@mariocarabotta , Rachel suggests we change the hover behaviour, so that we display the "Edit" button only when hovering the image (not the whole row).
The assumption is that the user will discover this easily enough.

I'm happy with that, what do you think?

@mariocarabotta mariocarabotta added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Jan 16, 2024
@mariocarabotta
Copy link
Collaborator

@mariocarabotta , Rachel suggests we change the hover behaviour, so that we display the "Edit" button only when hovering the image (not the whole row). The assumption is that the user will discover this easily enough.

I'm happy with that, what do you think?

sounds good!

@mariocarabotta
Copy link
Collaborator

mariocarabotta commented Jan 16, 2024

@mariocarabotta , Rachel suggests we change the hover behaviour, so that we display the "Edit" button only when hovering the image (not the whole row). The assumption is that the user will discover this easily enough.
I'm happy with that, what do you think?

sounds good!

@dacook also I'm thinking about people using it on mobile, probably better to make the whole image clickable for editing - basically the edit button is more of a hint for new users for learning that you can edit the image by clicking on it

With a new 'mini' button style.

For now, it's just a shortcut to the image edit page.
@dacook
Copy link
Member Author

dacook commented Jan 16, 2024

Thanks for confirming Mario. I've found it will be simpler to fix this in the next PR so will merge this one.

Have rebased again, with columns set properly.

@dacook dacook merged commit 20ca33a into openfoodfoundation:master Jan 16, 2024
50 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

6 participants