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

@W-14337462@ - Fix: Improper nesting of elements #1541

Merged
merged 41 commits into from
Nov 17, 2023

Conversation

joeluong-sfcc
Copy link
Collaborator

@joeluong-sfcc joeluong-sfcc commented Nov 15, 2023

Description

This PR fixes improper nesting structure in the product tile where screen readers were unable to properly read the content of a product tile due to the nesting of the wishlist button.

Before (Wishlist in readout)
Screenshot 2023-11-16 at 12 50 12 PM

After (Wishlist no longer in readout)

Screenshot 2023-11-16 at 12 53 22 PM

This PR also:

  • adds an aria label to the variant swatch groups on the product view component
  • adds translations to en-GB locale to dismiss missing translation warnings in console
  • adds meta title/description to PLP page for SEO for search results

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

  • restructure PLP tile to pull out wishlist button out of link element
  • add an aria label to the variant swatch groups on the product view component
  • add translations to en-GB locale to dismiss missing translation warnings in console
  • add meta title/description to PLP page for SEO for search results

How to Test-Drive This PR

  • With a screen reader, navigate to the PLP page and focus onto a product tile

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

Base automatically changed from ju/accessibility-form-fields to develop November 15, 2023 18:27
@joeluong-sfcc joeluong-sfcc marked this pull request as ready for review November 16, 2023 17:55
@joeluong-sfcc joeluong-sfcc requested a review from a team as a code owner November 16, 2023 17:55
<title>{category?.pageTitle}</title>
<meta name="description" content={category?.pageDescription} />
<title>{category?.pageTitle ?? searchQuery}</title>
<meta name="description" content={category?.pageDescription ?? searchQuery} />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

SEO lighthouse check was failing since PLP pages with search queries (non category) had no title/description in header

"header.button.assistive_msg.my_cart": {
"defaultMessage": "My cart"
"header.button.assistive_msg.my_cart_with_num_items": {
"defaultMessage": "My cart, number of items: {numItems}"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added translations for en-GB to clear Missing Translation warnings in console when developing

@alexvuong
Copy link
Collaborator

The highlight for product item is not visible.
image

@@ -14,7 +14,7 @@
"build-translations": "npm run extract-default-translations && npm run compile-translations && npm run compile-translations:pseudo",
"compile-translations": "node ./scripts/translations/compile-folder.js translations",
"compile-translations:pseudo": "node ./scripts/translations/compile-pseudo.js translations/en-US.json",
"extract-default-translations": "node ./scripts/translations/extract-default-messages.js en-US",
"extract-default-translations": "node ./scripts/translations/extract-default-messages.js en-US && node ./scripts/translations/extract-default-messages.js en-GB",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we use both en-US and en-GB for English, adding one more extraction command for en-GB can avoid the issue of missing translation for en-GB

@joeluong-sfcc
Copy link
Collaborator Author

The highlight for product item is not visible.

@alexvuong Do you mean allowing focus for the name/price? If so, I believe the current behavior should be fine. The product tile itself is highlighted when you focus over it, and the name/price get's read out with the screen reader. I think if we allow focus over the price/name we introduce redundancy in the readout

@alexvuong
Copy link
Collaborator

alexvuong commented Nov 17, 2023

The highlight for product item is not visible.

@alexvuong Do you mean allowing focus for the name/price? If so, I believe the current behavior should be fine. The product tile itself is highlighted when you focus over it, and the name/price get's read out with the screen reader. I think if we allow focus over the price/name we introduce redundancy in the readout

@joeluong-sfcc I think the blue highlight should be visible and wrap around the element that is currently read from the voice over. In the screenshot, there was only a small dot which does not look right. Look familiar to this issue: #1536

wjhsf
wjhsf previously approved these changes Nov 17, 2023
wjhsf
wjhsf previously approved these changes Nov 17, 2023
Copy link
Collaborator

@alexvuong alexvuong left a comment

Choose a reason for hiding this comment

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

LGTM

@joeluong-sfcc joeluong-sfcc merged commit faa38dc into develop Nov 17, 2023
17 checks passed
@joeluong-sfcc joeluong-sfcc deleted the fix/accessibility-nesting branch November 17, 2023 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants