Add product description title in old product editor #35154

louwie17 · 2022-10-18T13:34:09Z

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:

Added a product description title to the product text editor, using a similar format as the short description, making use of the postbox styling.
There wasn't an overly clean way to do this aside from doing it through JavaScript, as the edit-form-advanced.php file doesn't provide many filters to wrap the wp_editor in: https://github.com/WordPress/WordPress/blob/master/wp-admin/edit-form-advanced.php#L581-L649
The only options would be to make use of the edit_form_after_title and remove the post_type_supports, but this could break backwards compatibility and the edit_form_after_title does not seem like the right action to use for this.

One side note to my JavaScript implementation, I decided to only append the title html and add a class to handle the remaining styling. I did try to actually wrap the editor html using innerHtml, but this seemed to break the bootstrapping of the editor.

cc @jarekmorawski for styling approval, see above two images.

Closes #33538 .

How to test the changes in this Pull Request:

Load this branch and go to Products > Add New and notice how the long description is wrapped in a panel named Product Description
Do the same check for editing an existing product
Now download the Classic Editor plugin and edit an existing post or page and make sure they still look the same.

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 --filter=<project> run 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.

github-actions · 2022-10-18T14:01:22Z

Test Results Summary

Commit SHA: 0a72fc3

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	196	0	0	2	0	198	0m 55s
E2E Tests	186	0	0	3	0	189	19m 25s

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

Nice work! Flashbacks to the old days of jQuery.

Left one question and a minor clean-up comment, but pre-approving as everything is working as described.

The design feels slightly odd and eats into valuable screen real estate on smaller screens, but I'll leave this up to @jarekmorawski to approve.

joshuatf · 2022-10-19T23:06:59Z

plugins/woocommerce/client/legacy/css/admin.scss

+		margin: 6px 12px 0;
+	}
+	#post-status-info {
+		margin: 0px 12px 12px;


Do we have access to our usual scss vars in here? If so we could use gap vars like $gap-small.

Unfortunately it doesn't seem like we do.

joshuatf · 2022-10-19T23:07:07Z

plugins/woocommerce/client/legacy/js/admin/product-editor.js

@@ -0,0 +1,15 @@
+/* global woocommerce_admin_product_editor */
+jQuery( function ( $ ) {


jQuery 💥

joshuatf · 2022-10-19T23:09:49Z

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

+				wp_enqueue_script( 'wc-admin-product-editor', WC()->plugin_url() . '/assets/js/admin/product-editor' . $suffix . '.js', array( 'jquery' ), $version );
+
+				wp_localize_script( 'wc-admin-product-editor', 'woocommerce_admin_product_editor', array(
+					'i18n_description' => esc_js( __( 'Product Description', 'woocommerce' ) ),


I think this should be a lowercase "D" to match the other field titles on the page:

Suggested change

'i18n_description' => esc_js( __( 'Product Description', 'woocommerce' ) ),

'i18n_description' => esc_js( __( 'Product description', 'woocommerce' ) ),

jarekmorawski · 2022-10-20T06:49:35Z

cc @jarekmorawski for styling approval, see above two images.

Can we add a bottom padding to the permalink element, so the space between the product name input field and the description card is maintained? It looks off when they're this close to each other.

The design feels slightly odd and eats into valuable screen real estate on smaller screens, but I'll leave this up to @jarekmorawski to approve.

What about it feels odd to you, Josh? It does take up more space, but not unlike all other cards on the screen. I would keep it as is.

louwie17 · 2022-10-21T12:02:14Z

Thanks @jarekmorawski I updated the screenshots above with the new spacing.

Can we add a bottom padding to the permalink element, so the space between the product name input field and the description card is maintained? It looks off when they're this close to each other.

Instead of add a bottom padding to the permalink, I added a margin to the top of the description meta box. Mostly because this would prevent the Product description to jump down once the permalink becomes visible. This is how the previous design was laid out as well. The extra spacing doesn't appear to be to noticable.

louwie17 · 2022-10-21T12:02:31Z

@joshuatf this should be ready for a re-review

jarekmorawski · 2022-10-21T12:03:29Z

I agree. It looks good to me!

joshuatf

Sorry about the late re-review! LGTM 🚢

What about it feels odd to you, Josh? It does take up more space, but not unlike all other cards on the screen. I would keep it as is.

I suppose because unlike other cards it's an editing experience and on smaller screens could reduce the editor by almost 10%. But if this looks good to you, then it's fine by me. I also think the number of users editing on smaller screens is relatively small.

jarekmorawski · 2022-10-28T16:52:48Z

I suppose because unlike other cards it's an editing experience and on smaller screens could reduce the editor by almost 10%. But if this looks good to you, then it's fine by me. I also think the number of users editing on smaller screens is relatively small.

AFAIK it's in single digits. I don't think we need to focus on it too much now. We'll do mobile right in the new UX. 👍

github-actions · 2022-10-28T17:18:25Z

Hi @louwie17, 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

louwie17 requested a review from a team October 18, 2022 13:34

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

joshuatf previously approved these changes Oct 19, 2022

View reviewed changes

louwie17 added 4 commits October 21, 2022 08:29

Add product description title to classic product editor

80c9759

Remove bottom margin as its already added by the parent element

c351eb8

Add changelog

cb5d098

Update css and name, also fix some lint errors

1551091

louwie17 dismissed joshuatf’s stale review via 1551091 October 21, 2022 11:53

louwie17 force-pushed the update/33538_update_long_description_copy_and_layout branch from d5b624e to 1551091 Compare October 21, 2022 11:53

Fix spacing

0a72fc3

louwie17 requested a review from joshuatf October 25, 2022 12:10

joshuatf approved these changes Oct 28, 2022

View reviewed changes

louwie17 merged commit 6b6a332 into trunk Oct 28, 2022

louwie17 deleted the update/33538_update_long_description_copy_and_layout branch October 28, 2022 17:18

github-actions bot added this to the 7.2.0 milestone Oct 28, 2022

github-actions bot mentioned this pull request Dec 15, 2022

Test WooCommerce 7.2 UVLabs/location-picker-at-checkout-for-woocommerce#57

Closed

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

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

Closed

		@@ -0,0 +1,15 @@
		/* global woocommerce_admin_product_editor */
		jQuery( function ( $ ) {

	'i18n_description' => esc_js( __( 'Product Description', 'woocommerce' ) ),
	'i18n_description' => esc_js( __( 'Product description', 'woocommerce' ) ),

Add product description title in old product editor #35154

Add product description title in old product editor #35154

Uh oh!

Conversation

louwie17 commented Oct 18, 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 Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results Summary

Uh oh!

joshuatf left a comment

Choose a reason for hiding this comment

Uh oh!

joshuatf Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

louwie17 Oct 21, 2022

Choose a reason for hiding this comment

Uh oh!

joshuatf Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

joshuatf Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

jarekmorawski commented Oct 20, 2022

Uh oh!

louwie17 commented Oct 21, 2022

Uh oh!

louwie17 commented Oct 21, 2022

Uh oh!

jarekmorawski commented Oct 21, 2022

Uh oh!

joshuatf left a comment

Choose a reason for hiding this comment

Uh oh!

jarekmorawski commented Oct 28, 2022

Uh oh!

github-actions bot commented Oct 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

louwie17 commented Oct 18, 2022 •

edited

Loading

github-actions bot commented Oct 18, 2022 •

edited

Loading