-
Notifications
You must be signed in to change notification settings - Fork 143
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-12627117@ - [Accessibility][Checkout Page][Toggle Cart Actions] Adding aria-label for action buttons #1906
Conversation
packages/template-retail-react-app/app/components/toggle-card/index.jsx
Outdated
Show resolved
Hide resolved
@@ -152,6 +152,12 @@ const Payment = () => { | |||
} | |||
disabled={appliedPayment == null} | |||
onEdit={() => goToStep(STEPS.PAYMENT)} | |||
editLabel={ | |||
<FormattedMessage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A React Component is passed, but it is used as a component, which is kinda odd. Why not use intl.formatMessage and have the ToggleCard consumes as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah. Makes sense. Replaced it with formatMessage({...})
. Thanks!
@@ -34,6 +34,15 @@ export const ToggleCard = ({ | |||
children, | |||
...props | |||
}) => { | |||
let editButtonAriaLabel = 'Edit' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This string will need to have translation attached to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced it with formatMessage
editLabel?.type?.displayName === 'MemoizedFormattedMessage' | ||
) { | ||
editButtonAriaLabel = editLabel?.props.defaultMessage[0].value | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not necessary to have this if logic here, any reason we can't consume editLabel prop directly into the component as
// ToggleCart.jsx
...
<Button
variant="link"
size="sm"
onClick={onEdit}
aria-label={editLabel}
/>
...
// Payment Component
<ToggleCart editLabel={intl.formattedMessage({id: 'Payment Edit', defaultMessage: "Payment Edit"})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced logic passing formatMessage() to editLabel
variant="link" | ||
size="sm" | ||
onClick={onEdit} | ||
aria-label={editButtonAriaLabel} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test for this please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the small comment, LGTM
@@ -6,7 +6,7 @@ | |||
*/ | |||
import React, {useState} from 'react' | |||
import {nanoid} from 'nanoid' | |||
import {defineMessage, useIntl} from 'react-intl' | |||
import {FormattedMessage, defineMessage, useIntl} from 'react-intl' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FormattedMessage
doesn't seemed to be used in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed unused import
Description
[Issue] There are multiple buttons that have the same label of 'Edit' [User Impact] Screen reader users will have difficulty distinguishing between these buttons.
Types of Changes
Changes
How to Test-Drive This PR
npm start
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization