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-12627224@ Account menu a11y #1884

Merged
merged 12 commits into from
Jul 15, 2024
Merged

@W-12627224@ Account menu a11y #1884

merged 12 commits into from
Jul 15, 2024

Conversation

vcua-mobify
Copy link
Contributor

This PR contains a couple of a11y fixes for the account dropdown

  • Using a header instead of <p> tag on the account drop down menu header
  • Using box to indicate current selection in addition to color. This change also affects other menu-link items.
  • Using underline to indicate hover in addition to color. This change also affects other menu-link items.
  • Wrapping the menu items in a <li> tag

Desktop view changes: (Order History is active page, Account Details is hovered)
Screenshot 2024-07-08 at 5 16 49 PM

Mobile view changes: (Wishlist is active page, Order History is hovered)
Screenshot 2024-07-08 at 5 28 01 PM

Testing these changes:

  • Pull down these changes and start the app
  • Log in
  • Expand the 'My Account' drop down menu
  • Verify menu header is <h2> instead of <p>. Also verify that the text styling is the same as before
  • Verify each menu item is wrapped in a <li>
  • Verify that hovering over menu items now underlines the selection
  • Click a menu item
  • Verify the selected page is now indicated in the drop down menu as well as the left account page nav

@vcua-mobify vcua-mobify requested a review from a team as a code owner July 9, 2024 00:38
shethj
shethj previously approved these changes Jul 9, 2024
@alexvuong
Copy link
Collaborator

The border on the menu item looks a bit odd, was it intentional? Why does it only show border-top and border-bottom with round corners?

@vcua-mobify
Copy link
Contributor Author

vcua-mobify commented Jul 10, 2024

@alexvuong I replaced the borderBlock (which only shows top and bottom border) with border (which shows all 4 sides)

Screenshot 2024-07-10 at 4 52 06 PM

@@ -42,20 +42,24 @@ export default {
color: 'black',
justifyContent: 'flex-start',
fontSize: 'sm',
_hover: {bg: 'gray.50', textDecoration: 'none'},
_hover: {bg: 'gray.50', textDecoration: 'underline', textDecorationColor: '#181818'},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
_hover: {bg: 'gray.50', textDecoration: 'underline', textDecorationColor: '#181818'},
_hover: {bg: 'gray.50', textDecoration: 'underline', textDecorationColor: 'gray.900'},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestion applied. I also replaced the blues with blue.600

@vcua-mobify vcua-mobify requested review from alexvuong and shethj July 11, 2024 16:35
joeluong-sfcc
joeluong-sfcc previously approved these changes Jul 12, 2024
@vcua-mobify
Copy link
Contributor Author

After discussions with the UI team, we have settled on using a left border bar as shown in this screen shot

Screenshot 2024-07-15 at 11 25 40 AM

alexvuong
alexvuong previously approved these changes Jul 15, 2024
joeluong-sfcc
joeluong-sfcc previously approved these changes Jul 15, 2024
@vcua-mobify vcua-mobify merged commit 67ba9c8 into develop Jul 15, 2024
28 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.

4 participants