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

Fix/37601 Add aria-describedby to Select options button #37880

Merged
merged 9 commits into from May 12, 2023

Conversation

faisal-alvi
Copy link
Member

Submission Review Guidelines:

Changes proposed in this Pull Request:

  • This PR adds a function to get the aria-describedby description for the add to cart button.
  • Also adds default description for the Select options button.

Closes #37601.

How to test the changes in this Pull Request:

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

  1. Go to the Shop page.
  2. Inspect the button "Select options"
  3. Check the aria-describedby attribute is added with the value: "This product has multiple variants. The options may be chosen on the product page".

image

@github-actions github-actions bot added plugin: woocommerce Issues related to the WooCommerce Core plugin. type: community contribution labels Apr 20, 2023
@woocommercebot woocommercebot requested review from a team and jorgeatorres and removed request for a team April 20, 2023 10:11
Copy link
Member

@jorgeatorres jorgeatorres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @faisal-alvi!

Thanks for your contribution 💯. I tested and while this works when the default "Select options" string is used, it doesn't really work if that string changes (for example, when translated).

I left some comments on how to possibly fix that, as well as some general (though easily solved) feedback.

Please let me know what you think. Looking forward to hearing back from you.

'rel' => 'nofollow',
),
);

$args = apply_filters( 'woocommerce_loop_add_to_cart_args', wp_parse_args( $args, $defaults ), $product );

if ( isset( $args['attributes']['aria-describedby'] ) && ! empty( $args['attributes']['aria-describedby'] ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to check that the key is set with isset(). empty() already takes care of that.

plugins/woocommerce/includes/wc-template-functions.php Outdated Show resolved Hide resolved
@jorgeatorres jorgeatorres added the needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. label May 8, 2023
@faisal-alvi
Copy link
Member Author

@jorgeatorres thanks for the feedback. The suggestions/changes are implemented. Please check 99b5c7e and let me know if any changes are required.

@jorgeatorres jorgeatorres self-requested a review May 12, 2023 14:56
@github-actions
Copy link
Contributor

Hi ,

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

@jorgeatorres jorgeatorres force-pushed the fix/37601 branch 2 times, most recently from 70177d1 to 98d2fb9 Compare May 12, 2023 17:58
@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #37880 (1b04c2f) into trunk (924c871) will decrease coverage by 0.0%.
The diff coverage is 5.9%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             trunk   #37880     +/-   ##
==========================================
- Coverage     51.5%    51.5%   -0.0%     
- Complexity   17281    17443    +162     
==========================================
  Files          430      440     +10     
  Lines        80030    80290    +260     
==========================================
+ Hits         41217    41346    +129     
- Misses       38813    38944    +131     
Impacted Files Coverage Δ
...ommerce/includes/abstracts/abstract-wc-product.php 88.0% <0.0%> (-0.3%) ⬇️
.../woocommerce/includes/admin/wc-admin-functions.php 41.3% <0.0%> (ø)
...woocommerce/includes/class-wc-product-variable.php 75.7% <0.0%> (-0.7%) ⬇️
.../woocommerce/includes/class-wc-structured-data.php 0.0% <0.0%> (ø)
...ins/woocommerce/includes/wc-template-functions.php 12.2% <0.0%> (+0.1%) ⬆️
plugins/woocommerce/includes/wc-core-functions.php 63.6% <100.0%> (ø)

... and 41 files with indirect coverage changes

Copy link
Member

@jorgeatorres jorgeatorres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you @faisal-alvi!

(I just made a tiny change to address a PHPCS issue, but other than that, it looked good)

@jorgeatorres jorgeatorres merged commit 8e9ff0d into woocommerce:trunk May 12, 2023
16 of 19 checks passed
@github-actions github-actions bot added this to the 7.8.0 milestone May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contributor Day - H1 2023 needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. plugin: woocommerce Issues related to the WooCommerce Core plugin. type: community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessibility Improvements ‣ Shop ‣ Action Buttons
2 participants