Images Product management MVP 1.0 #34769

octaedro · 2022-09-21T19:38:21Z

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

This PR adds the Image section to the product creation/edition.

Detail:

It adds an Images section.
It adds the upload image component and a gallery to see and order the images.

The following functionalities are not included here, but they will be added in a follow-up PR.

Clicking anywhere on the card (except for buttons) opens the file picker.
The addition of a toolbar from the Gallery block in Gutenberg to manage gallery images.

Closes #34759.

How to test the changes in this Pull Request:

Checkout this branch.
Grab one (or more) image from your computer and drop it in the Drag images here or click to upload area.
Verify that the image has been uploaded correctly and is now being shown on the screen.
Verify that the Drag images here or click to upload area is smaller when there is at least one image loaded.
Upload another image and change the order of the images.
Then save the product. Verify that the new order for the images is saved correctly.
Press Choose images and select an image that has been uploaded before.
Verify that the selected image is added to the image gallery component.
Grab an image from the gallery and verify that the area Drop here to remove is now visible.
Drop the image in that area and verify that the image is removed from the gallery.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully run tests with your changes locally?
Have you created a changelog file for each project being changed, ie pnpm changelog add --filter=<project>?

FOR PR REVIEWER ONLY:

I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

github-actions · 2022-09-21T20:08:16Z

Test Results Summary

Commit SHA: d64d689

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	188	0	0	2	0	190	0m 46s
E2E Tests	127	4	2	8	0	141	11m 31s

⚠️ Warning

Please address the following issues prior to merging this pull request:

FAILED/BROKEN TESTS. There were failed and/or broken API and E2E tests.
INCOMPLETE E2E TEST RUN. We have a total of 189 E2E tests, but only 141 were executed. Note that in CI, E2E tests automatically end when they encounter too many failures.

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

joshuatf

Very nice implementation, @octaedro! Left a few suggestions on where we can move logic.

joshuatf · 2022-09-26T15:44:28Z

packages/js/components/src/image-gallery/image-gallery.tsx

 export const ImageGallery: React.FC< ImageGalleryProps > = ( {
 	children,
 	columns = 4,
+	onDragStart = () => null,


Nice! Thanks for adding these in. Should we add onDragOver as well?

Good idea! I added it in the commit 58c89b2

joshuatf · 2022-09-26T15:51:52Z

plugins/woocommerce-admin/client/products/sections/images-section.scss

+	&__images {
+		margin-bottom: 0;
+		box-shadow: unset;
+		.woocommerce-media-uploader {


Should any of these styles be moved to the MediaUploader component? Or are they specific to the product form?

Good suggestion! I moved a few lines to the package in commit a44aaa6. I left the image and the code related to that in the client because it was something specific to this implementation.

joshuatf · 2022-09-26T15:52:10Z

plugins/woocommerce-admin/client/products/sections/images-section.scss

+		}
+	}
+	&__images.has-images {
+		.woocommerce-media-uploader {


Same question here.

I would leave this code here, since these styles will be applied depending on whether the Card that contains the component MediaUploader and ImageGallery has a class or not.

Referring to the has-images class? This could be something useful inside that component as well.

But I guess the real question is do we want similar behavior to this in most of our other media uploader components.

Good idea, if it's ok with you I'll tackle this in a follow-up PR.

joshuatf · 2022-09-26T15:52:53Z

plugins/woocommerce-admin/client/products/sections/images-section.scss

+				margin-top: 0;
+			}
+		}
+		.woocommerce-sortable {


Same question here. (under the ImageGallery component I would image)

Since these styles also depend on whether the parent component has a class or not, I would leave these styles here.

joshuatf · 2022-09-26T15:54:17Z

plugins/woocommerce-admin/client/products/sections/images-section.tsx

+							recordEvent( 'prepare_images_help' );
+						} }
+					>
+						{ __( 'How to prepare images?', 'woocommerce' ) }


This seems like it might be a sentence fragment. Should we change it to How to prepare images without the ? or How should I prepare images??

Nice catch! I changed the copy in commit 2cf569d.

joshuatf · 2022-09-26T16:06:47Z

plugins/woocommerce-admin/client/products/sections/images-section.tsx

+		setShowRemoveZone( ! showRemoveZone );
+	};
+
+	const setImageToRemove = ( image: string ) => {


Instead of setting the image to remove later, could we just setValue( 'images', newImages ); here after filtering out the removed?

Yeah, that's a good idea, and it was my first thought too. The problem I found was that it would remove the image before triggering the onDragEnd here.
That method (among other things) removes the class is-draggin in the Sortable component. So it would create a few oddities.

joshuatf · 2022-09-26T16:16:23Z

plugins/woocommerce-admin/client/products/sections/images-section.tsx

+	const setImageToRemove = ( image: string ) => {
+		const tempNode = document.createElement( 'div' );
+		tempNode.innerHTML = image;
+		const imageId = tempNode.getElementsByTagName( 'img' )[ 0 ].id;


I think instead of retrieving this ID via selectors, we could set the ID of the dragged element on start. For example:

onDragStart={ ( image ) => setDraggedImageId( image.key ) }

I'm not sure if I understood this suggestion correctly. When a user grabs an image, we don't know if the user is wanting to modify the order of the images or remove that image. If the user wants only to change the order of the images, we wouldn't need to save that value.

It's okay if they don't end up using the draggedImageId; we can set it to null later. But at least this way we can just used the ID stored in state instead of using dom query selectors.

Got it! I like the idea! I changed it in commit ddccb5b

joshuatf · 2022-09-26T16:16:56Z

plugins/woocommerce-admin/client/products/sections/images-section.tsx

+								key={ image.id }
+								alt={ image.alt }
+								src={ image.url || image.src }
+								id={ `${ image.id }` }


Note that we could forego the ID prop here and just opt to use the key to keep props down if we implement my above suggestion.

Right now we're not presenting the key when we render an ImageGalleryItem item. The rendered id is what we use to pair our list when the items are reordered or to delete an image.

joshuatf · 2022-09-26T16:22:49Z

plugins/woocommerce-admin/client/products/sections/images-section.tsx

+	};
+
+	const orderImages = ( newOrder: JSX.Element[] ) => {
+		const orderedImages = newOrder.map( ( image ) => {


Is it possible to just directly call setValue( 'images', newOrder );? Does the data differ pretty significantly?

I'd consider this a shortcoming in the onOrderChange callback if so. Not a blocker for this PR if that's the case, but would love to open an issue to make this easier.

That is not possible now. The onOrderChange method returns the items and we are saving another kind of object under images.
I think that it would be a great addition to make the method onOrderChange return the same type of object we will use later.

Great idea! I've been looking on feedback on this and @louwie17 had a similar request.

octaedro · 2022-09-28T19:30:52Z

Thank you @joshuatf for your review!
I just addressed the changes you suggested and answered the questions you asked.
Could you take another look at this PR?

joshuatf

@octaedro I'm having trouble getting this branch started again. I'm seeing the old TS errors that we were hitting before the latest fixes in trunk. Could you give this a rebase?

joshuatf · 2022-10-04T14:59:33Z

plugins/woocommerce-admin/client/products/sections/images-section.scss

+		}
+	}
+	&__images.has-images {
+		.woocommerce-media-uploader {


Referring to the has-images class? This could be something useful inside that component as well.

But I guess the real question is do we want similar behavior to this in most of our other media uploader components.

joshuatf · 2022-10-04T16:04:23Z

plugins/woocommerce-admin/client/products/sections/images-section.tsx

+	};
+
+	const orderImages = ( newOrder: JSX.Element[] ) => {
+		const orderedImages = newOrder.map( ( image ) => {


Great idea! I've been looking on feedback on this and @louwie17 had a similar request.

joshuatf · 2022-10-04T16:06:02Z

plugins/woocommerce-admin/client/products/sections/images-section.tsx

+export const ImagesSection: React.FC = () => {
+	const { getInputProps, setValue } = useFormContext< Product >();
+	const images = ( getInputProps( 'images' ).value as Image[] ) || [];
+	const [ showRemoveZone, setShowRemoveZone ] = useState( false );


Nitpicky but isRemoveZoneVisible to avoid prefixing this with a verb.

Good idea! I renamed the const in the commit 662d29c

joshuatf · 2022-10-04T16:19:29Z

plugins/woocommerce-admin/client/products/sections/images-section.tsx

+	const [ showRemoveZone, setShowRemoveZone ] = useState( false );
+	const [ imageIdToRemove, setImageIdToRemove ] = useState< number >( 0 );
+
+	const getUniqueImages = ( files: Image[] ) => {


It would be great to extract this to a utils file so we can both test it and minimize the logic in this component.

I like this, if it is ok with you I'll move the method in the same follow-up PR to refactor the code.

joshuatf · 2022-10-04T17:03:29Z

plugins/woocommerce-admin/client/products/sections/images-section.tsx

+	const setImageToRemove = ( image: string ) => {
+		const tempNode = document.createElement( 'div' );
+		tempNode.innerHTML = image;
+		const imageId = tempNode.getElementsByTagName( 'img' )[ 0 ].id;


It's okay if they don't end up using the draggedImageId; we can set it to null later. But at least this way we can just used the ID stored in state instead of using dom query selectors.

joshuatf · 2022-10-04T21:27:56Z

plugins/woocommerce-admin/client/products/sections/images-section.tsx

+				images.push( image );
+			}
+		} );
+		setValue( 'images', images );


This getUniqueImages function has a side effect of setValue which feels out of place. We're also calling setValue below again after calling getUniqueImages.

setValue( 'images', getUniqueImages( [ file ] ) )

Nice catch! I removed the setValue( 'images', images ); inside the function in commit 5ceceaa

joshuatf · 2022-10-04T21:30:24Z

plugins/woocommerce-admin/client/products/sections/images-section.tsx

+		files.forEach( ( image: Image ) => {
+			if (
+				image.id &&
+				images.find( ( file ) => file.id === image.id ) === undefined


I'm having some trouble understanding what the purpose of the getUniqueImages function really does. Is this just to guarantee the same image is not selected twice?

What happens if someone selects the same image twice? Nothing happens?

If we really need this, we might also want to consider storing a memoized version of images so we don't have nested loops.

Yeah, I think that we really don't need the method getUniqueImages. I removed it in commit 372dea0

joshuatf

Fantastic work, @octaedro! The image section is testing really well for me. Love the work on the deletion drop zone.

Left a couple very minor comments, but pre-approving.

I think this PR might have an outdated lock file, but feel free to ping me on Slack for re-approval as soon as that's updated 😄

joshuatf · 2022-10-11T23:29:18Z

plugins/woocommerce-admin/client/products/sections/images-section.tsx

+		useState< boolean >( false );
+	const [ isRemove, setIsRemove ] = useState< boolean >( false );
+	const [ draggedImageId, setDraggedImageId ] = useState< number | null >(
+		0


Should this initially be null?

Good call! I changed it in the commit 15b342d

joshuatf · 2022-10-11T23:29:32Z

plugins/woocommerce-admin/client/products/sections/images-section.tsx

+						onDragStart={ ( event ) => {
+							const { id: imageId, dataset } = event.target as HTMLElement;
+							if ( imageId ) {
+								setDraggedImageId( parseInt( imageId, 10 ) );


Nicely done!

joshuatf · 2022-10-11T23:30:12Z

plugins/woocommerce-admin/client/products/sections/images-section.tsx

+							const { id: imageId, dataset } = event.target as HTMLElement;
+							if ( imageId ) {
+								setDraggedImageId( parseInt( imageId, 10 ) );
+							} else {


Just curious - what scenario do we not have imageId and end up inside this else?

That happens when the image to move is grabbed by the icon in the toolbar. At that point, we don't have the image id, and this is why we use the image index (in the images list), to get the image id.

Ah I see. Thanks for the explanation!

plugins/woocommerce-admin/client/products/sections/images-section.tsx

# Conflicts: # plugins/woocommerce-admin/client/products/add-product-page.tsx

# Conflicts: # packages/js/components/src/sortable/sortable-item.tsx # packages/js/components/src/sortable/sortable.tsx # Conflicts: # packages/js/components/src/sortable/sortable.tsx

# Conflicts: # packages/js/components/src/image-gallery/image-gallery-item.tsx # packages/js/components/src/image-gallery/image-gallery.tsx

# Conflicts: # packages/js/components/src/sortable/sortable-item.tsx # packages/js/components/src/sortable/sortable.tsx # Conflicts: # packages/js/components/src/image-gallery/image-gallery.tsx # packages/js/components/src/sortable/sortable.tsx

…efox

…ion.tsx Co-authored-by: Joshua T Flowers <[email protected]>

joshuatf

Testing well! Thanks for minor fix ups and the explanation around the imageId!

github-actions · 2022-10-12T18:16:37Z

Hi @octaedro, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

Add the release: add testing instructions label

octaedro changed the title ~~Add/39 image section details~~ Images Product management MVP 1.0 Sep 22, 2022

octaedro force-pushed the add/39_image_section_details branch from 1cea4c0 to 0052fda Compare September 22, 2022 15:57

github-actions bot removed the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Sep 22, 2022

octaedro requested a review from a team September 22, 2022 21:20

octaedro marked this pull request as ready for review September 22, 2022 21:20

octaedro mentioned this pull request Sep 26, 2022

Add initial attributes section for empty state #34751

Merged

8 tasks

joshuatf reviewed Sep 26, 2022

View reviewed changes

octaedro force-pushed the add/39_image_section_details branch from 7b05648 to a13f75d Compare September 28, 2022 18:53

octaedro requested a review from joshuatf September 30, 2022 00:43

joshuatf reviewed Oct 4, 2022

View reviewed changes

octaedro force-pushed the add/39_image_section_details branch 2 times, most recently from 46f1585 to cfabfe8 Compare October 6, 2022 16:43

github-actions bot added the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Oct 6, 2022

octaedro force-pushed the add/39_image_section_details branch from 73f73f1 to 215065f Compare October 7, 2022 19:34

github-actions bot removed the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Oct 7, 2022

octaedro requested a review from joshuatf October 10, 2022 14:47

joshuatf previously approved these changes Oct 11, 2022

View reviewed changes

octaedro added 7 commits October 12, 2022 09:47

Add image section

35b169e

# Conflicts: # plugins/woocommerce-admin/client/products/add-product-page.tsx

Add keepSpaceWhenDragging to sortable

c732d77

# Conflicts: # packages/js/components/src/sortable/sortable-item.tsx # packages/js/components/src/sortable/sortable.tsx # Conflicts: # packages/js/components/src/sortable/sortable.tsx

Export ImageGalleryItem

7aa48bf

Add props to image-gallery

5c0fdcd

# Conflicts: # packages/js/components/src/image-gallery/image-gallery-item.tsx # packages/js/components/src/image-gallery/image-gallery.tsx

Changed media-uploader label

d13b9ac

Add changelogs

bd0345a

Fix image-gallery and sortable components

e56677c

# Conflicts: # packages/js/components/src/sortable/sortable-item.tsx # packages/js/components/src/sortable/sortable.tsx # Conflicts: # packages/js/components/src/image-gallery/image-gallery.tsx # packages/js/components/src/sortable/sortable.tsx

octaedro and others added 16 commits October 12, 2022 10:02

Add CardBody and remove redundant setValue

ffccd95

Fix card styles

ff01183

Remove getUniqueImages

8e03875

Use url as a key when there is no image id

cc5eff4

Fix orderedImages

cee88a3

Add hover to gallery images

8c303a7

Fix gallery arrows and set cover problems

1faca64

Altering blur handler to prevent toolbar closure on media modal

f3e2800

Fix toolbar drag and drop

3dece2d

Add replace fn

b04450a

Restoring modal class for blur function

cf92208

Fix storybook

f1a26f4

Ensuring onBlur doesn't happen while dragging, resolving issue in Fir…

c7f9cfc

…efox

Adding expected event object to drag callbacks

a8e2911

Fix image size

23ab884

Fix lint

d6cdafe

octaedro dismissed joshuatf’s stale review via d6cdafe October 12, 2022 14:10

octaedro force-pushed the add/39_image_section_details branch from 215065f to d6cdafe Compare October 12, 2022 14:10

octaedro and others added 6 commits October 12, 2022 11:30

Another fix lint

60fbc19

Update plugins/woocommerce-admin/client/products/sections/images-sect…

f467deb

…ion.tsx Co-authored-by: Joshua T Flowers <[email protected]>

Fix draggedImageId default value

15b342d

Fix toolbar icon style

643642a

Rename consts

4c22581

Update pnpm-lock.yaml

d64d689

joshuatf approved these changes Oct 12, 2022

View reviewed changes

octaedro merged commit 98162b9 into trunk Oct 12, 2022

octaedro deleted the add/39_image_section_details branch October 12, 2022 18:16

github-actions bot added this to the 7.1.0 milestone Oct 12, 2022

github-actions bot mentioned this pull request Nov 10, 2022

Test WooCommerce 7.1 UVLabs/location-picker-at-checkout-for-woocommerce#55

Closed

Images Product management MVP 1.0 #34769

Images Product management MVP 1.0 #34769

Uh oh!

Conversation

octaedro commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

FOR PR REVIEWER ONLY:

Uh oh!

github-actions bot commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results Summary

⚠️ Warning

Uh oh!

joshuatf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octaedro Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octaedro commented Sep 28, 2022

Uh oh!

joshuatf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

octaedro commented Sep 21, 2022 •

edited

Loading

github-actions bot commented Sep 21, 2022 •

edited

Loading

octaedro Sep 28, 2022 •

edited

Loading