Introduce missing button element classes for the account orders and downloads #37933

xristos3490 · 2023-04-21T16:20:25Z

Submission Review Guidelines:

I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have reviewed my code for security best practices.
Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Before starting ensure that you are using a block theme like TT2, or TT3.

Testing in order-again context:

Create a new order and get to the Order Received page (don't close it)
Process the order from wp-admin and refresh the previous page to see the "Order again" button.
Notice that it's missing some styles. See https://d.pr/i/oVXIas
Apply this fix, and ensure the button appears correctly.

Testing in the pagination buttons on the account page:

Navigate to My Account > Orders page and ensure that you have enough so the pagination shows up (hint: one could use the woocommerce_my_account_my_orders_query filter to wire a small number (e.g., 2) in the limit key for the query)
Notice the buttons are missing styles. See https://d.pr/i/lYs1jg
Apply this fix, and ensure the buttons appear correctly.

Testing in the no-downloads cta button on the account page:

Navigate to My Account > Downloads page and ensure your user hasn't downloaded yet.
Check the CTA button inside the notice saying "No downloads available yet"; it shouldn't look right.
Apply this patch and ensure the CTA shows up correctly.

github-actions · 2023-04-21T16:43:36Z

Test Results Summary

Commit SHA: f1b1df9

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	267	0	0	2	0	269	1m 0s
E2E Tests	189	0	0	10	0	199	21m 11s

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.

codecov · 2023-04-21T16:53:55Z

Codecov Report

Merging #37933 (f0ab1de) into trunk (6ed8fb4) will increase coverage by 0.0%.
The diff coverage is 0.0%.

❗ Current head f0ab1de differs from pull request most recent head f1b1df9. Consider uploading reports for the commit f1b1df9 to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##             trunk   #37933   +/-   ##
========================================
  Coverage     51.2%    51.2%           
+ Complexity   17439    17417   -22     
========================================
  Files          440      440           
  Lines        80722    80636   -86     
========================================
- Hits         41346    41312   -34     
+ Misses       39376    39324   -52

Impacted Files	Coverage Δ
...ins/woocommerce/includes/wc-template-functions.php	`12.2% <0.0%> (-<0.1%)`	⬇️

... and 10 files with indirect coverage changes

github-actions · 2023-05-04T07:13:55Z

Hi @coreymckrill,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

github-actions · 2023-05-04T07:13:56Z

Hi @coreymckrill,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

xristos3490 · 2023-05-04T07:31:29Z

Hey @coreymckrill! fyi I left the template bumps so we can decide on which version these will land. I can then push another commit containing the proper version number.

coreymckrill

@xristos3490 👍 looks good, other than the needed fix on the order again template 😅

This PR also needs a rebase since it was originally submitted before the monorepo upgraded pnpm to v8.

coreymckrill · 2023-05-04T23:11:32Z

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

 			'order/order-again.php',
 			array(
 				'order'           => $order,
+				'wp_button_class' => wc_wp_theme_get_element_class_name( 'button' ) ? ' ' . wc_wp_theme_get_element_class_name( 'button' ) : '',


Suggested change

'wp_button_class' => wc_wp_theme_get_element_class_name( 'button' ) ? ' ' . wc_wp_theme_get_element_class_name( 'button' ) : '',

'wp_button_class' => wc_wp_theme_get_element_class_name( 'button' ),

Optional, non-blocking suggestion, but I would argue for having this template parameter only contain the classname, and worry about the whitespace in the template itself. And also, probably not worry about extra whitespace at the end of the class attribute in the case that wp_button_class is empty.

I was inspired by other instances of the same fix throughout the codebase (like here)

To be honest, I also believe that the templates should be responsible for managing the classes. Ideally, I'd expect to echo them using arrays and the implode function. We could move forward with this as is, progress with other changes in regards to the block-ification of the templates, and then return to create a more organized solution.

I'd be happy to change it, though. Thoughts?

Ideally, I'd expect to echo them using arrays and the implode function. We could move forward with this as is, progress with other changes in regards to the block-ification of the templates, and then return to create a more organized solution.

👍 Yeah, doing this separately sounds like the right approach. No need to make this PR more complicated.

plugins/woocommerce/templates/order/order-again.php

Co-authored-by: Corey McKrill <[email protected]>

…woocommerce into add/tt3-comp-button-classes

xristos3490 · 2023-05-05T12:42:53Z

Thanks for the review, @coreymckrill! I've updated the base of this fix.

coreymckrill

Bah, I approved this and then realized that the code sniff test is failing. Should be good to go after that's fixed.

xristos3490 · 2023-05-08T19:31:45Z

Nice catch, @coreymckrill! 🙇

I have made some adjustments to ensure PHPCS is satisfied. However, we still need to determine the appropriate version number for the template modifications. What do you think? Would 7.7.0 be a suitable choice, or perhaps 7.6.2?

coreymckrill

we still need to determine the appropriate version number for the template modifications. What do you think? Would 7.7.0 be a suitable choice, or perhaps 7.6.2?

This doesn't seem like an urgent bug fix that would qualify for a point release (let me know if you disagree). Given that 7.7.0 is now out (sorry, was at a conference), I think 7.8.0 would be the right target here.

xristos3490 · 2023-05-12T11:16:39Z

I think 7.8.0 would be the right target here.

Totally agree! Changing templates is always a good idea to do it, at least, on a minor release! Will update the headers shortly!

xristos3490 · 2023-05-15T13:39:40Z

Updated the template version and rebased the branch.

Two templates have already been upgraded to 7.8.0. So, the Highlight templates changes is probably a false positive.

coreymckrill · 2023-05-16T00:18:39Z

Thanks @xristos3490 !

xristos3490 added 4 commits April 21, 2023 19:02

Add order-again button class

65bf738

Add no-downloads button class

43bd317

Add order-pagination button class

ddb399b

Add changelog

e30983e

github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Apr 21, 2023

xristos3490 mentioned this pull request Apr 21, 2023

Fix TT2 button styles #37579

Closed

xristos3490 requested review from a team and coreymckrill and removed request for a team May 4, 2023 07:13

coreymckrill suggested changes May 5, 2023

View reviewed changes

xristos3490 and others added 3 commits May 5, 2023 15:31

Merge branch 'trunk' into add/tt3-comp-button-classes

c55af48

Fix typo for echoing the class name

dab5f98

Co-authored-by: Corey McKrill <[email protected]>

Merge branch 'add/tt3-comp-button-classes' of github.com:woocommerce/…

f0ab1de

…woocommerce into add/tt3-comp-button-classes

coreymckrill approved these changes May 5, 2023

View reviewed changes

coreymckrill suggested changes May 5, 2023

View reviewed changes

xristos3490 added 2 commits May 8, 2023 21:50

Merge branch 'trunk' into add/tt3-comp-button-classes

f43b943

Make phpcs happy

92287f7

coreymckrill previously approved these changes May 10, 2023

View reviewed changes

xristos3490 added 2 commits May 15, 2023 14:23

Merge branch 'trunk' into add/tt3-comp-button-classes

ee74bc6

Bump template versions

66c5a16

xristos3490 dismissed coreymckrill’s stale review via 66c5a16 May 15, 2023 11:24

Bump order-again template

f1b1df9

coreymckrill approved these changes May 16, 2023

View reviewed changes

coreymckrill merged commit 5a3e0b1 into trunk May 16, 2023

coreymckrill deleted the add/tt3-comp-button-classes branch May 16, 2023 00:18

github-actions bot added this to the 7.8.0 milestone May 16, 2023

xristos3490 self-assigned this May 31, 2023

	'wp_button_class' => wc_wp_theme_get_element_class_name( 'button' ) ? ' ' . wc_wp_theme_get_element_class_name( 'button' ) : '',
	'wp_button_class' => wc_wp_theme_get_element_class_name( 'button' ),

Introduce missing button element classes for the account orders and downloads #37933

Introduce missing button element classes for the account orders and downloads #37933

Uh oh!

Conversation

xristos3490 commented Apr 21, 2023

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Uh oh!

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

Uh oh!

Test Results Summary

Uh oh!

codecov bot commented Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented May 4, 2023

Uh oh!

github-actions bot commented May 4, 2023

Uh oh!

xristos3490 commented May 4, 2023

Uh oh!

coreymckrill left a comment

Choose a reason for hiding this comment

Uh oh!

coreymckrill May 4, 2023

Choose a reason for hiding this comment

Uh oh!

xristos3490 May 5, 2023

Choose a reason for hiding this comment

Uh oh!

coreymckrill May 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xristos3490 commented May 5, 2023

Uh oh!

coreymckrill left a comment

Choose a reason for hiding this comment

Uh oh!

xristos3490 commented May 8, 2023

Uh oh!

coreymckrill left a comment

Choose a reason for hiding this comment

Uh oh!

xristos3490 commented May 12, 2023

Uh oh!

xristos3490 commented May 15, 2023

Uh oh!

coreymckrill commented May 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Apr 21, 2023 •

edited

Loading

codecov bot commented Apr 21, 2023 •

edited

Loading