Skip to content
This repository has been archived by the owner on Aug 30, 2018. It is now read-only.

[Timber] update a11y prices #564

Closed
wants to merge 2 commits into from
Closed

Conversation

matcaissy
Copy link
Contributor

@matcaissy matcaissy commented Aug 12, 2016

Fix for https://github.com/Shopify/shopify-themes/issues/3523

Did some cleanup - and organized better the a11y prices for screenreaders on the product grid, product and search page.

Demo: link

@ruairiphackett 馃憖
cc @cshold

@matcaissy matcaissy self-assigned this Aug 12, 2016
@@ -243,8 +245,11 @@ timber.productPage = function (options) {
$comparePrice
.html({{ 'products.product.compare_at' | t | json }} + ' ' + Shopify.formatMoney(variant.compare_at_price, moneyFormat))
.show();
$comparePriceA11y.attr('aria-hidden', 'false');
$priceA11y.attr('aria-hidden', 'false');
Copy link
Contributor

Choose a reason for hiding this comment

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

Does $priceA11y never get turned back to aria-hidden="true"?

Copy link

@ruairiphackett ruairiphackett Aug 22, 2016

Choose a reason for hiding this comment

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

My understanding is that $priceA11y should never be set to aria-hidden="true" as it should always be 'visible' to the screen reader/VO, is that correct?

Setting aria-hidden to false may not be needed though, would not setting any value to the aria-hidden attribute have the same effect as setting it to false here?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's never aria-hidden=true you don't have to set the attribute at all. Unset is the same as "false"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's never aria-hidden=true you don't have to set the attribute at all. Unset is the same as "false"

Agreed. I missed that out.

@matcaissy
Copy link
Contributor Author

matcaissy commented Aug 22, 2016

@cshold @ruairiphackett did another fix in this commit: 124079d

Updated demo: link

@cshold
Copy link
Contributor

cshold commented Aug 24, 2016

Can you update that link, it's not Timber anymore

@ruairiphackett
Copy link

The changes look good @matcaissy 馃憤

I don't think the changes are present on the updated demo link, but I tested them on my own test store and they look good 馃帺

@matcaissy
Copy link
Contributor Author

Sorry folks, here's the demo: link

@ruairiphackett
Copy link

Looks good, just wondering about one thing, there's 2 labels on the compare at price on the product page when it is present:

Apologies for messy gif!

http://recordit.co/GRdNkDKNTd.gif

Do we need both the regular_price and compare_at labels here?

@matcaissy
Copy link
Contributor Author

Closing down, will revisit themes a11y in a new project (Q2?)

@matcaissy matcaissy closed this Jan 24, 2017
@matcaissy matcaissy deleted the timber-update-a11y-prices branch January 24, 2017 18:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants