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

Change all ARIA labels to translatable messages #1572

Merged
merged 22 commits into from
Dec 8, 2023
Merged

Conversation

wjhsf
Copy link
Contributor

@wjhsf wjhsf commented Nov 28, 2023

Description

Noticed when reviewing #1570 that the ARIA labels were hard-coded strings, rather than translatable messages. Did a quick search in the project to find all the other instances of that. Spotted a few small issues along the way, so I fixed those as well.

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

  • Listed in commit history

How to Test-Drive This PR

To test that the messages are now translatable:

  • Update relevant translations in translation files
  • Start dev server
  • Observe that your changes are reflected on the page

To test other changes

  • Start dev server
  • Click "My account" button as guest. Should open login modal.
  • Click "My account" button as registered user. Should go to account.
  • Hitting space/enter on "My account" button should have the same result as clicking.
  • Hitting space/enter on account menu icon should open the menu.
  • Turn on VoiceOver
  • View login screen.
  • Tab over the 👁️‍🗨️ icon. Should say "show password".
  • Click 👁️‍🗨️ icon. Should show password and now say "hide password".
  • Tab focus on "My account" button. It should now say "My account, button"
    • It should probably be a link, rather than a button, but that's harder to change without breaking customers
  • Tab focus on account dropdown icon. It should now say "Open account menu"

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)

@wjhsf wjhsf requested a review from a team as a code owner November 28, 2023 20:28
const path = buildUrl('/account')
history.push(path)
} else {
// if they already are at the login page, do not show login modal
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was a bug reported from UX awhile ago saying if a user is current in login page, the account click should not show the modal since user is already seeing the login form

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That bug is also handled by the withRegistration component. Although I noticed that here the logic handled /login, and the withRegistration component handles /{locale}/login, but what the app actually uses is /global/{locale}/login. I've changed the withRegistration component to be more flexible.

wjhsf and others added 5 commits November 29, 2023 10:27
Signed-off-by: Will Harney <62956339+wjhsf@users.noreply.github.com>
withRegistration component detects /{locale}/login, but app uses /global/{locale}/login and /login.

const handleClick = (e) => {
e.preventDefault()
if (!customer.isRegistered) {
// Do not show auth modal if users is already on the login page
if (isLoginPage) {
if (isLoginPage(location, locale)) {
Copy link
Collaborator

@alexvuong alexvuong Nov 29, 2023

Choose a reason for hiding this comment

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

is locale being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not here, but it was in the original implementation, so I figured we'd keep it to make customizations easier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is only one place that defines the isLoginPage which does not take locale. I don't think we need to do it here. 🤔 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Locale was used previously:

const isLoginPage = new RegExp(`^/${locale}/login$`).test(location.pathname)

Because our default implementation has multiple login pages (/login, /en-GB/login, /global/en-GB/login), I decided to change the isLoginPage logic. But, to make it easier for customers to implement the previous logic (or any locale-dependent custom logic), I think it's reasonable to also provide locale.

wjhsf and others added 2 commits November 29, 2023 14:41
Signed-off-by: Will Harney <62956339+wjhsf@users.noreply.github.com>
@wjhsf wjhsf enabled auto-merge (squash) November 30, 2023 15:12
alexvuong
alexvuong previously approved these changes Nov 30, 2023
@wjhsf wjhsf merged commit d9d27a7 into develop Dec 8, 2023
20 checks passed
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.

None yet

3 participants