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 checkout header logo navigation #670

Merged
merged 6 commits into from
Aug 5, 2022
Merged

Conversation

alexvuong
Copy link
Collaborator

@alexvuong alexvuong commented Jul 16, 2022

Description

The CheckoutHeader is using the wrong navigation for logo navigation. It has not taken into account the link for multi-site.

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

  • (change1)

How to Test-Drive This PR

  • npm run start at the retail react app
  • Go to localhost:3000/us/en-US/checkout
  • Click on the logo icon, it should navigate the users back to localhost:3000/us/en-US

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)

@alexvuong alexvuong requested a review from a team as a code owner July 16, 2022 00:12
</Link>
<IconButton
aria-label={intl.formatMessage({
id: 'header.button.assistive_msg.logo',
Copy link
Collaborator

Choose a reason for hiding this comment

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

The message has been changed from "Back to hompage" to "Logo". I think the "Logo" take makes sense when we used it to describe the ui that didn't have any action associated to it, but in this case we might want to stick to the original message.

If you agree, you can make a follow up localization ticket to get the translation for that text if there isn't already one created.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, assistive text should describe what an element does, not what it looks like.

Please also keep this as a link, rather than a button. Buttons make the behavior opaque and lack critical accessibility features associated with links.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reverted the button to be a link and made a small adjustment to link component to work with home ref insteaf. No need to tweak any translations

wjhsf
wjhsf previously requested changes Jul 19, 2022
</Link>
<IconButton
aria-label={intl.formatMessage({
id: 'header.button.assistive_msg.logo',
Copy link
Contributor

Choose a reason for hiding this comment

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

+1, assistive text should describe what an element does, not what it looks like.

Please also keep this as a link, rather than a button. Buttons make the behavior opaque and lack critical accessibility features associated with links.

@alexvuong alexvuong requested review from wjhsf and bendvc August 4, 2022 23:54
@@ -19,7 +19,7 @@ const Link = React.forwardRef(({href, to, useNavLink = false, ...props}, ref) =>
as={useNavLink ? NavSPALink : SPALink}
{...(useNavLink && {exact: true})}
{...props}
to={_href === '/' ? '/' : updatedHref}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With multi-site implementation in place, the condition to keep '/' as is out of date. When the href is a '/', it should also be passed to buildUrl to generate the correct homepage url according to current site and locale

@alexvuong alexvuong added the ready for review PR is ready to be reviewed label Aug 5, 2022
@alexvuong alexvuong dismissed wjhsf’s stale review August 5, 2022 17:05

I've addressed the issue

@alexvuong alexvuong merged commit 3396472 into develop Aug 5, 2022
@wjhsf wjhsf deleted the fix/fix-checkout-header branch March 17, 2023 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants