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

feat!: added Menu component family using Reach UI #1163

Closed
wants to merge 24 commits into from
Closed

Conversation

JoshuaKGoldberg
Copy link
Collaborator

@JoshuaKGoldberg JoshuaKGoldberg commented Nov 17, 2020

Overview

PR Checklist

Description

Directly uses a large portion of Reach UI's Menu Button family, with the addition of a MenuButtonToggle for the chevron icon expando logic.
Slack thread on avoiding Popover directly

Renames the old SideMenu component's MenuItem to SideMenuItem to distinguish it from the MenuItem here.

See also https://reach.tech/menu-button for now the menu button normally behaves. I was surprised that you can't use tabbing inside it when open, just up/down arrow keys...

Toggling the menu popover

@JoshuaKGoldberg JoshuaKGoldberg changed the title feat: added Menu component family using Reach UI feat!: added Menu component family using Reach UI Nov 17, 2020
@codecov
Copy link

codecov bot commented Nov 17, 2020

Codecov Report

Merging #1163 (0be9535) into main (13c8826) will increase coverage by 0.17%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1163      +/-   ##
==========================================
+ Coverage   69.18%   69.36%   +0.17%     
==========================================
  Files         283      289       +6     
  Lines        2233     2246      +13     
  Branches      784      786       +2     
==========================================
+ Hits         1545     1558      +13     
+ Misses        678      677       -1     
- Partials       10       11       +1     
Impacted Files Coverage Δ
packages/gamut/src/SideMenus/SideMenu/index.tsx 50.00% <ø> (ø)
packages/gamut/src/Menu/MenuButton.tsx 100.00% <100.00%> (ø)
packages/gamut/src/Menu/MenuButtonToggle.tsx 100.00% <100.00%> (ø)
packages/gamut/src/Menu/MenuItem.tsx 100.00% <100.00%> (ø)
packages/gamut/src/Menu/MenuLink.tsx 100.00% <100.00%> (ø)
packages/gamut/src/Menu/MenuList.tsx 100.00% <100.00%> (ø)
packages/gamut/src/Menu/styles.ts 100.00% <100.00%> (ø)
...ackages/gamut/src/SideMenus/SideMenuItem/index.tsx 100.00% <100.00%> (ø)
packages/gamut-labs/src/brand/ProLogo/index.tsx 75.00% <0.00%> (ø)
... and 5 more

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review November 19, 2020 22:07
@JoshuaKGoldberg JoshuaKGoldberg requested a review from a team as a code owner November 19, 2020 22:07
import { listItemStyles } from './styles';

export const MenuLink = styled(ReachMenuLink)`
${listItemStyles}
Copy link
Contributor

Choose a reason for hiding this comment

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

ooh didn't know we can style this way! thx for teaching me

@jcenglish
Copy link
Contributor

I think it'd be helpful for the submenus' a tags to have aria-expanded='true' and aria-expanded='false' attributes when they're open and closed. That way, screenreader users know for sure it's a button that reveals more navigation options and not some other kind of button with unpredictable behavior. That's how it is in this W3 example.

Comment on lines 16 to 18
&[data-focus-visible-added] {
box-shadow: 0 0 0 2px ${colors.black};
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@codecademydev
Copy link
Collaborator

📬Published Alpha Packages:

@codecademy/gamut-labs@5.7.3-alpha.0be953.0
@codecademy/gamut@17.5.0-alpha.0be953.0
@codecademy/styleguide@18.11.0-alpha.0be953.0

@codecademydev
Copy link
Collaborator

🚀 Styleguide deploy preview ready!

https://5fbe51349261666cdb8de86c--gamut-preview.netlify.app

Deploy Logs

@JoshuaKGoldberg
Copy link
Collaborator Author

Closing to keep my PR queue small.

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