Add help tip for Product Image and Product Gallery meta boxes #35834

nathanss · 2022-12-05T13:52:06Z

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:

Adds a tooltip for both "Set product image" and "Add product gallery images" buttons

Closes #35017.

This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

Open Add/Edit product page.
Verify the updated UI of the Product Image and Product Gallery meta boxes

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 created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?

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.

nathanss · 2022-12-05T14:28:18Z

I feel that the jQuery implementation is not ideal, since you can see the tooltip showing up after the initial render.

However, I couldn't find a good alternative using the wc_product_post_thumbnail_html filter, since it's just a string and I can't just append to it, as it needs to show to the right of the Link, instead of at the bottom. It would be nice to have some way of parsing the string as DOM elements in PHP. Is there any?

Also, I added the tooltip text twice. Is there any way of improving that?

github-actions · 2022-12-05T14:48:41Z

Test Results Summary

Commit SHA: 24a87c4

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 55s
E2E Tests	187	0	0	6	0	193	14m 52s

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.

mattsherman

Nice start on this!

I left a few minor comments. Once the PR is out of Draft, just ping me and I'll review it again/officially!

Not a big deal, but I noticed that if you click "Set product image", select an image, and then remove that image, the tooltip after "Set product image" doesn't re-appear. If easy to fix, I'd do it. Otherwise, it's fine as is.

mattsherman · 2022-12-05T18:06:43Z

plugins/woocommerce/includes/admin/class-wc-admin-assets.php

 					'i18n_product_other_tip'             => __( 'Product types define available product details and attributes, such as downloadable files and variations. They’re also used for analytics and inventory management.', 'woocommerce' ),
 					'i18n_product_description_tip'       => __( 'Describe this product. What makes it unique? What are its most important features?', 'woocommerce' ),
 					'i18n_product_short_description_tip' => __( 'Summarize this product in 1-2 short sentences. We’ll show it at the top of the page.', 'woocommerce' ),
+					'i18n_product_image_tip'             => __( 'For best results, upload JPEG or PNG files that are 1000 by 1000 pixels or larger. Maximum upload file size: 2GB.', 'woocommerce' ),


The max upload size is dynamic. Use:

size_format( wp_max_upload_size() )

mattsherman · 2022-12-05T18:07:52Z

plugins/woocommerce/includes/admin/meta-boxes/class-wc-meta-box-product-images.php

 		</div>
 		<p class="add_product_images hide-if-no-js">
 			<a href="#" data-choose="<?php esc_attr_e( 'Add images to product gallery', 'woocommerce' ); ?>" data-update="<?php esc_attr_e( 'Add to gallery', 'woocommerce' ); ?>" data-delete="<?php esc_attr_e( 'Delete image', 'woocommerce' ); ?>" data-text="<?php esc_attr_e( 'Delete', 'woocommerce' ); ?>"><?php esc_html_e( 'Add product gallery images', 'woocommerce' ); ?></a>
+			<?php echo wc_help_tip( __('For best results, upload JPEG or PNG files that are 1000 by 1000 pixels or larger. Maximum upload file size: 2GB.', 'woocommerce' ) ) ?>


Can you use the i18n_product_image_tip in class-wc-admin-assets.php above?

I don't know how to do that. Can you help me find a way?

Ha, nevermind... 😄 I failed to notice that the i18n_product_image_tip was actually getting put into a JS script. Hmm... I guess the only ways to avoid this duplication would be to...

move this out to a separate global that could be references by both class-wc-admin-assets.php and this php file, or

move the creation of this tooltip to JS as the other one is being done (though there is no other reason for it to be done in JS)

I don't hate option (2)... it brings a certain consistency to how they are both handled.

mattsherman · 2022-12-05T18:15:51Z

plugins/woocommerce/client/legacy/js/admin/woocommerce_admin.js

+			.append(
+				'<span class="woocommerce-help-tip" tabindex="-1"></span>'
+			)
+			.find( '.woocommerce-help-tip' )


I'm not a jQuery expert (I looked this up), but I think you could do...

$( '#set-post-thumbnail` ).after( '' )

That would be a little more clearer, I think.

mattsherman · 2022-12-05T18:16:19Z

plugins/woocommerce/client/legacy/js/admin/woocommerce_admin.js

+			.attr( 'for', 'content' )
+			.attr(
+				'aria-label',
+				woocommerce_admin_meta_boxes.i18n_product_image_tip
+			)


Any reason not to just include those in the initial markup for the span?

Good one. I copied the code from another place. I tested moving the attributes directly to the markup and it seems to work.

mattsherman · 2022-12-05T18:46:46Z

plugins/woocommerce/client/legacy/js/admin/woocommerce_admin.js

 			$( this ).trigger( 'focus' );
 		} );

+		$( '#set-post-thumbnail' )


Since the #set-post-thumbnail element is added via a filter, and thus could be altered by another plugin, we should account for that and handle the scenario where this element doesn't exist.

nathanss · 2022-12-05T19:35:09Z

@mattsherman I'm also thinking that, even though I'm adding aria-label for the tooltip in jQuery, the one generated in PHP by wc_help_tip does not contain it. Should I make both of them have aria-label, or perhaps remove from the jQuery one?

nathanss · 2022-12-05T22:51:01Z

@mattsherman Another thought: what if I add them both via jQuery? Since I was able to only add one of them via PHP. Might make the whole solution simpler.

mattsherman · 2022-12-06T00:21:51Z

@mattsherman I'm also thinking that, even though I'm adding aria-label for the tooltip in jQuery, the one generated in PHP by wc_help_tip does not contain it. Should I make both of them have aria-label, or perhaps remove from the jQuery one?

We definitely want the aria-label.

Another thought: what if I add them both via jQuery? Since I was able to only add one of them via PHP. Might make the whole solution simpler.

I'm leaning heavily towards thinking this is best, especially if the wc_help_tip doesn't support aria-label.

mattsherman

Nice iteration. I agree with you that perhaps you should just do both thumbnails in JS for consistency.

mattsherman · 2022-12-05T23:51:59Z

plugins/woocommerce/includes/admin/class-wc-admin-assets.php

 					'i18n_product_other_tip'             => __( 'Product types define available product details and attributes, such as downloadable files and variations. They’re also used for analytics and inventory management.', 'woocommerce' ),
 					'i18n_product_description_tip'       => __( 'Describe this product. What makes it unique? What are its most important features?', 'woocommerce' ),
 					'i18n_product_short_description_tip' => __( 'Summarize this product in 1-2 short sentences. We’ll show it at the top of the page.', 'woocommerce' ),
+					/* translators: %s: maximum file size */


%s should be %1$s

mattsherman · 2022-12-05T23:52:10Z

plugins/woocommerce/includes/admin/meta-boxes/class-wc-meta-box-product-images.php

 			<a href="#" data-choose="<?php esc_attr_e( 'Add images to product gallery', 'woocommerce' ); ?>" data-update="<?php esc_attr_e( 'Add to gallery', 'woocommerce' ); ?>" data-delete="<?php esc_attr_e( 'Delete image', 'woocommerce' ); ?>" data-text="<?php esc_attr_e( 'Delete', 'woocommerce' ); ?>"><?php esc_html_e( 'Add product gallery images', 'woocommerce' ); ?></a>
-			<?php echo wc_help_tip( sprintf( __( 'For best results, upload JPEG or PNG files that are 1000 by 1000 pixels or larger. Maximum upload file size: %1$s.', 'woocommerce' ), size_format( wp_max_upload_size() ) ) ); ?>
+			<?php
+			/* translators: %s: maximum file size */


%s should be %1$s

mattsherman · 2022-12-06T00:15:44Z

plugins/woocommerce/includes/admin/meta-boxes/class-wc-meta-box-product-images.php

 		</div>
 		<p class="add_product_images hide-if-no-js">
 			<a href="#" data-choose="<?php esc_attr_e( 'Add images to product gallery', 'woocommerce' ); ?>" data-update="<?php esc_attr_e( 'Add to gallery', 'woocommerce' ); ?>" data-delete="<?php esc_attr_e( 'Delete image', 'woocommerce' ); ?>" data-text="<?php esc_attr_e( 'Delete', 'woocommerce' ); ?>"><?php esc_html_e( 'Add product gallery images', 'woocommerce' ); ?></a>
+			<?php echo wc_help_tip( __('For best results, upload JPEG or PNG files that are 1000 by 1000 pixels or larger. Maximum upload file size: 2GB.', 'woocommerce' ) ) ?>


Ha, nevermind... 😄 I failed to notice that the i18n_product_image_tip was actually getting put into a JS script. Hmm... I guess the only ways to avoid this duplication would be to...

move this out to a separate global that could be references by both class-wc-admin-assets.php and this php file, or

move the creation of this tooltip to JS as the other one is being done (though there is no other reason for it to be done in JS)

I don't hate option (2)... it brings a certain consistency to how they are both handled.

mattsherman · 2022-12-06T00:27:15Z

plugins/woocommerce/client/legacy/js/admin/woocommerce_admin.js

+			setPostThumbnail
+				.after(
+					`<span class="woocommerce-help-tip" tabindex="-1" for="content" aria-label="${ woocommerce_admin_meta_boxes.i18n_product_image_tip }"></span>`
+				)
+				.parent()
+				.find( '.woocommerce-help-tip' )


The whole .parent().find() was bugging me, so I dug a bit...

you can do this instead...

$( `` ) .insertAfter( setPostThumbnail ) .tipTip()

thanks! that's better

mattsherman · 2022-12-06T00:45:26Z

I feel that the jQuery implementation is not ideal, since you can see the tooltip showing up after the initial render.

Yeah, it's a little janky, but it doesn't seem too bad. To be honest, there is a lot of post-render re-layout going on in this page, so this is just a minor addition to existing problems (which we are addressing in the new product experience).

However, I couldn't find a good alternative using the wc_product_post_thumbnail_html filter, since it's just a string and I can't just append to it, as it needs to show to the right of the Link, instead of at the bottom. It would be nice to have some way of parsing the string as DOM elements in PHP. Is there any?

Well, you could load the string into a DOMDocument instance and manipulate it. Not sure what the performance implication would be for this. We'd want to make sure it doesn't slow down the rendering of the page. If it performs well, it could be the way to go, to keep everything in PHP.

See https://stackoverflow.com/questions/5126967/extract-dom-elements-from-string-in-php

Though, it does seem to leave us without aria-label support if we go that route, which is less than ideal (though, not a new issue... we have other tooltip using wc_help_tip that suffer from this... you know, if would be quite easy, and a nice improvement to WC Core, to just update wc_help_tip to set the aria-label (and the for too). That feels like a safe improvement we could make.

woocommerce/plugins/woocommerce/includes/wc-core-functions.php

Line 1594 in abcd2a7

function wc_help_tip( $tip, $allow_html = false ) {

Also, I added the tooltip text twice. Is there any way of improving that?

I gave some thoughts on this in another comment I recently made (I had overlooked your initial questions).

mattsherman · 2022-12-06T00:58:49Z

plugins/woocommerce/client/legacy/js/admin/woocommerce_admin.js

+		if ( setPostThumbnail ) {
+			setPostThumbnail
+				.after(
+					`<span class="woocommerce-help-tip" tabindex="-1" for="content" aria-label="${ woocommerce_admin_meta_boxes.i18n_product_image_tip }"></span>`


Is for a valid attribute for a ?

I only see a reference to it being used on <label>.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label#attr-for

Actually, since the aria-label is for the  itself, I don't think we need a for in any case...

The example of aria-label on MDN shows it being used on a <button> that does not have text content. That is the same case we have here.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-label

Refactor jQuery code for simplicity

Refactor DOM manipulation code Remove PHP implementation

mattsherman

Nice iteration on this, @nathanss ! I like the consistent approach you arrived at for both tooltips -- it's nice seeing less code and "duplication".

Tested and works well. Tried keyboard and VoiceOver and the tooltips were spoken when focused.

mattsherman

Nice find on why the e2e tests were failing, and I agree that this new location for this code seems like a better fit.

Re-tested and things still work well.

Just left one comment requesting a comment be added to describe what is being done (as is the convention in the rest of the file).

plugins/woocommerce/client/legacy/js/admin/meta-boxes-product.js

mattsherman

Nice work on this! Thanks for adding the comment

nathanss marked this pull request as draft December 5, 2022 13:52

github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Dec 5, 2022

mattsherman suggested changes Dec 5, 2022

View reviewed changes

mattsherman reviewed Dec 5, 2022

View reviewed changes

nathanss marked this pull request as ready for review December 5, 2022 19:33

nathanss requested a review from mattsherman December 5, 2022 19:47

mattsherman suggested changes Dec 6, 2022

View reviewed changes

mattsherman reviewed Dec 6, 2022

View reviewed changes

nathanss requested a review from mattsherman December 6, 2022 15:27

nathanss added 11 commits December 6, 2022 14:15

Add help tip to Product galery meta box

883b23b

Changelog

687cdce

Remove admin_post_thumbnail_html filter from product screen

d19accf

Add jQuery to append a tooltip after 'Set product image'

e9cfbd6

Add dynamic upload size

39e25c2

Add guard in case setPostThumbnail doesn't exist

b0d7b4a

Refactor jQuery code for simplicity

Run phpcbf

d140cda

Fix php linting

11b8a75

Run lint again

28d4f7f

Make both tooltips with jQuery

f54c4e2

Refactor DOM manipulation code Remove PHP implementation

Change span tabIndex to 0 to allow focus on screen readers

21d926e

nathanss force-pushed the update/35017 branch from a3e9b45 to 21d926e Compare December 6, 2022 17:15

mattsherman previously approved these changes Dec 6, 2022

View reviewed changes

Move code to meta-boxes-product

ae6ad99

nathanss dismissed mattsherman’s stale review via ae6ad99 December 6, 2022 22:04

mattsherman suggested changes Dec 6, 2022

View reviewed changes

plugins/woocommerce/client/legacy/js/admin/meta-boxes-product.js Show resolved Hide resolved

Add descriptive comment on jQuery code that adds tooltips

24a87c4

mattsherman approved these changes Dec 7, 2022

View reviewed changes

nathanss merged commit 493faac into trunk Dec 7, 2022

nathanss deleted the update/35017 branch December 7, 2022 14:38

github-actions bot added this to the 7.3.0 milestone Dec 7, 2022

github-actions bot mentioned this pull request Jan 13, 2023

Test WooCommerce 7.3 UVLabs/location-picker-at-checkout-for-woocommerce#59

Closed

Add help tip for Product Image and Product Gallery meta boxes #35834

Add help tip for Product Image and Product Gallery meta boxes #35834

Uh oh!

Conversation

nathanss commented Dec 5, 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!

nathanss commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Test Results Summary

Uh oh!

mattsherman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsherman Dec 5, 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!

nathanss commented Dec 5, 2022

Uh oh!

nathanss commented Dec 5, 2022

Uh oh!

mattsherman commented Dec 6, 2022

Uh oh!

mattsherman 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsherman commented Dec 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsherman left a comment

Choose a reason for hiding this comment

Uh oh!

mattsherman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattsherman left a comment • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

nathanss commented Dec 5, 2022 •

edited

Loading

nathanss commented Dec 5, 2022 •

edited

Loading

github-actions bot commented Dec 5, 2022 •

edited

Loading

mattsherman Dec 5, 2022 •

edited

Loading

mattsherman left a comment •

edited

Loading