Skip to content

Commit df154f3

Browse files
committed
Critique on Menu.
Note: Hamburger menu is still broken and need fixing.
1 parent f3c9ee8 commit df154f3

21 files changed

Lines changed: 601 additions & 120 deletions

.github/copilot-instructions.md

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
# Copilot Instructions
2+
3+
## Design Context
4+
5+
### Users
6+
Developers building consumer-facing SaaS products and web portals with React. They consume the component library as a foundation and layer their own brand on top. They care deeply about accessibility, predictability, and not fighting the library.
7+
8+
### Brand Personality
9+
**Professional · Cozy · Warm · Clean**
10+
11+
The library should feel trustworthy and polished without being cold or sterile. Components should radiate quiet competence — approachable enough that adopters feel comfortable, structured enough that their apps feel credible.
12+
13+
### Aesthetic Direction
14+
**Neutral & Adaptive** — the library stays out of the way.
15+
16+
Components use CSS custom properties throughout so adopters can retheme with minimal effort. The default palette (blue primary, light/dark surface tones) is a safe, professional baseline — not a statement. Every visual choice should be a sensible default, not a brand decision.
17+
18+
Anti-references: avoid anything flashy, opinionated, or hard to override — no gradient text, no neon dark-mode accents, no signature glassmorphism that would clash with adopters' own aesthetics.
19+
20+
### Design Principles
21+
1. **Accessibility is non-negotiable** — every component ships WCAG 2.2 AA compliant by default. Keyboard navigation, focus indicators, ARIA semantics, and color contrast are baseline requirements, not enhancements.
22+
2. **Tokens over hardcoded values** — all visual decisions (color, spacing, typography, radius, shadow) flow through CSS custom properties. Nothing is magic-numbered in component styles.
23+
3. **Predictable over clever** — components behave exactly as developers expect. Consistent prop patterns, standard HTML semantics, and no surprising side effects.
24+
4. **Progressive disclosure** — components expose simple APIs by default; power-user options (controlled state, callbacks, refs) are available but never required.
25+
5. **Motion respects the user** — all animations honor `prefers-reduced-motion`. Transitions are functional (communicating state), never decorative.

.impeccable.md

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
# .impeccable.md
2+
3+
## Design Context
4+
5+
### Users
6+
Developers building consumer-facing SaaS products and web portals with React. They consume the component library as a foundation and layer their own brand on top. They care deeply about accessibility, predictability, and not fighting the library.
7+
8+
### Brand Personality
9+
**Professional · Cozy · Warm · Clean**
10+
11+
The library should feel trustworthy and polished without being cold or sterile. Components should radiate quiet competence — approachable enough that adopters feel comfortable, structured enough that their apps feel credible.
12+
13+
### Aesthetic Direction
14+
**Neutral & Adaptive** — the library stays out of the way.
15+
16+
Components use CSS custom properties throughout so adopters can retheme with minimal effort. The default palette (blue primary, light/dark surface tones) is a safe, professional baseline — not a statement. Every visual choice should be a sensible default, not a brand decision.
17+
18+
Anti-references: avoid anything flashy, opinionated, or hard to override — no gradient text, no neon dark-mode accents, no signature glassmorphism that would clash with adopters' own aesthetics.
19+
20+
### Design Principles
21+
1. **Accessibility is non-negotiable** — every component ships WCAG 2.2 AA compliant by default. Keyboard navigation, focus indicators, ARIA semantics, and color contrast are baseline requirements, not enhancements.
22+
2. **Tokens over hardcoded values** — all visual decisions (color, spacing, typography, radius, shadow) flow through CSS custom properties. Nothing is magic-numbered in component styles.
23+
3. **Predictable over clever** — components behave exactly as developers expect. Consistent prop patterns, standard HTML semantics, and no surprising side effects.
24+
4. **Progressive disclosure** — components expose simple APIs by default; power-user options (controlled state, callbacks, refs) are available but never required.
25+
5. **Motion respects the user** — all animations honor `prefers-reduced-motion`. Transitions are functional (communicating state), never decorative.

package-lock.json

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/components/HamburgerMenu/HamburgerMenu.module.scss

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
.wrapper {
44
display: block;
55

6-
@include breakpoint-lg {
6+
@include breakpoint-md {
77
display: none;
88
}
99
}
@@ -39,6 +39,11 @@
3939
inset: 0;
4040
background: var(--color-overlay);
4141
z-index: var(--z-index-dialog);
42+
animation: fadeIn var(--transition-fast) ease forwards;
43+
44+
@include reduced-motion {
45+
animation: none;
46+
}
4247
}
4348

4449
.panel {
@@ -53,13 +58,33 @@
5358
display: flex;
5459
flex-direction: column;
5560
overflow-y: auto;
56-
transform: translateX(0);
61+
animation: slideIn var(--transition-normal) ease forwards;
5762

5863
@include reduced-motion {
5964
animation: none;
6065
}
6166
}
6267

68+
@keyframes slideIn {
69+
from {
70+
transform: translateX(-100%);
71+
}
72+
73+
to {
74+
transform: translateX(0);
75+
}
76+
}
77+
78+
@keyframes fadeIn {
79+
from {
80+
opacity: 0;
81+
}
82+
83+
to {
84+
opacity: 1;
85+
}
86+
}
87+
6388
.panelHeader {
6489
display: flex;
6590
justify-content: flex-end;
@@ -71,10 +96,11 @@
7196
flex: 1;
7297
overflow-y: auto;
7398

74-
/* Override SideMenu display:none so it shows inside hamburger panel */
99+
/* Override SideMenu breakpoint styles inside the hamburger slide-in panel */
75100
:global {
76101
nav {
77-
display: block !important;
102+
display: flex !important;
103+
flex-direction: column !important;
78104
width: 100% !important;
79105
border-right: none !important;
80106
}

src/components/HamburgerMenu/HamburgerMenu.stories.tsx

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import type { Meta, StoryObj } from '@storybook/react-vite';
22
import { HamburgerMenu } from './HamburgerMenu';
3+
import { SideMenu } from '../SideMenu';
34
import { MenuItem } from '../MenuItem';
45
import { MenuItemGroup } from '../MenuItemGroup';
56

@@ -17,16 +18,14 @@ export const Default: Story = {
1718
render: () => (
1819
/* style override ensures the hamburger is visible at any viewport in Storybook */
1920
<HamburgerMenu style={{ display: 'block' }}>
20-
<nav>
21-
<ul style={{ listStyle: 'none', padding: 0, margin: 0 }}>
22-
<MenuItem href="/dashboard" active>Dashboard</MenuItem>
23-
<MenuItem href="/analytics">Analytics</MenuItem>
24-
<MenuItemGroup label="Settings">
25-
<MenuItem href="/profile">Profile</MenuItem>
26-
<MenuItem href="/security">Security</MenuItem>
27-
</MenuItemGroup>
28-
</ul>
29-
</nav>
21+
<SideMenu style={{ display: 'flex' }}>
22+
<MenuItem href="/dashboard" active>Dashboard</MenuItem>
23+
<MenuItem href="/analytics">Analytics</MenuItem>
24+
<MenuItemGroup label="Settings">
25+
<MenuItem href="/profile">Profile</MenuItem>
26+
<MenuItem href="/security">Security</MenuItem>
27+
</MenuItemGroup>
28+
</SideMenu>
3029
</HamburgerMenu>
3130
),
3231
};

src/components/HamburgerMenu/HamburgerMenu.tsx

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
} from 'react';
1010
import { CloseButton } from '../CloseButton';
1111
import styles from './HamburgerMenu.module.scss';
12+
import { HamburgerMenuContext } from './HamburgerMenuContext';
1213

1314
export interface HamburgerMenuProps extends HTMLAttributes<HTMLDivElement> {
1415
/** Ref forwarded to the container. */
@@ -46,6 +47,7 @@ export function HamburgerMenu({
4647
const isControlled = controlledOpen !== undefined;
4748
const isOpen = isControlled ? controlledOpen : uncontrolledOpen;
4849
const panelRef = useRef<HTMLDivElement | null>(null);
50+
const toggleRef = useRef<HTMLButtonElement | null>(null);
4951

5052
const setOpen = useCallback(
5153
(next: boolean) => {
@@ -55,7 +57,10 @@ export function HamburgerMenu({
5557
[isControlled, onOpenChange],
5658
);
5759

58-
const close = useCallback(() => setOpen(false), [setOpen]);
60+
const close = useCallback(() => {
61+
setOpen(false);
62+
toggleRef.current?.focus();
63+
}, [setOpen]);
5964
const toggle = useCallback(() => setOpen(!isOpen), [isOpen, setOpen]);
6065

6166
/* Escape key */
@@ -100,6 +105,7 @@ export function HamburgerMenu({
100105
return (
101106
<div ref={ref} className={wrapperClasses} {...rest}>
102107
<button
108+
ref={toggleRef}
103109
type="button"
104110
className={styles.toggle}
105111
aria-expanded={isOpen}
@@ -135,7 +141,9 @@ export function HamburgerMenu({
135141
<CloseButton aria-label="Close menu" onClick={close} />
136142
</div>
137143
<div className={styles.panelContent}>
138-
{children}
144+
<HamburgerMenuContext.Provider value={true}>
145+
{children}
146+
</HamburgerMenuContext.Provider>
139147
</div>
140148
</div>
141149
</>
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
import { createContext, useContext } from 'react';
2+
3+
/** Set to true inside a HamburgerMenu panel to prevent SideMenu auto-collapse. */
4+
export const HamburgerMenuContext = createContext(false);
5+
6+
export function useInsideHamburgerMenu(): boolean {
7+
return useContext(HamburgerMenuContext);
8+
}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
11
export { HamburgerMenu } from './HamburgerMenu';
22
export type { HamburgerMenuProps } from './HamburgerMenu';
3+
export { HamburgerMenuContext } from './HamburgerMenuContext';
34

src/components/MenuItem/MenuItem.module.scss

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,23 +27,25 @@
2727
@include focus-ring;
2828

2929
&:hover {
30-
background-color: var(--color-surface);
30+
background-color: var(--color-surface-raised);
3131
}
3232

3333
&:active {
34-
background-color: var(--color-surface-raised);
34+
background-color: var(--color-surface-sunken, var(--color-surface-raised));
3535
}
3636
}
3737

3838
.active .link {
3939
color: var(--color-primary);
4040
background-color: var(--color-surface);
4141
font-weight: var(--font-weight-medium);
42+
box-shadow: inset 3px 0 0 var(--color-primary);
4243
}
4344

4445
.disabled .link {
4546
color: var(--color-text-disabled);
4647
cursor: not-allowed;
48+
pointer-events: none;
4749

4850
&:hover {
4951
background-color: transparent;
@@ -63,3 +65,13 @@
6365
@include truncate;
6466
}
6567

68+
.collapsed .link {
69+
justify-content: center;
70+
padding-left: var(--space-2);
71+
padding-right: var(--space-2);
72+
}
73+
74+
.collapsed .label {
75+
display: none;
76+
}
77+

src/components/MenuItem/MenuItem.test.tsx

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,40 +4,45 @@ import { MenuItem } from './MenuItem';
44

55
describe('MenuItem', () => {
66
it('renders as a link when href is provided', () => {
7-
render(<ul role="menu"><MenuItem href="/home">Home</MenuItem></ul>);
8-
expect(screen.getByRole('menuitem', { name: 'Home' })).toBeInTheDocument();
9-
expect(screen.getByRole('menuitem').tagName).toBe('A');
7+
render(<ul><MenuItem href="/home">Home</MenuItem></ul>);
8+
expect(screen.getByRole('link', { name: 'Home' })).toBeInTheDocument();
9+
expect(screen.getByRole('link', { name: 'Home' }).tagName).toBe('A');
1010
});
1111

1212
it('renders as a button when href is omitted', () => {
13-
render(<ul role="menu"><MenuItem>Settings</MenuItem></ul>);
14-
expect(screen.getByRole('menuitem').tagName).toBe('BUTTON');
13+
render(<ul><MenuItem>Settings</MenuItem></ul>);
14+
expect(screen.getByRole('button', { name: 'Settings' }).tagName).toBe('BUTTON');
1515
});
1616

1717
it('sets aria-current="page" when active', () => {
18-
render(<ul role="menu"><MenuItem href="/home" active>Home</MenuItem></ul>);
19-
expect(screen.getByRole('menuitem')).toHaveAttribute('aria-current', 'page');
18+
render(<ul><MenuItem href="/home" active>Home</MenuItem></ul>);
19+
expect(screen.getByRole('link', { name: 'Home' })).toHaveAttribute('aria-current', 'page');
2020
});
2121

2222
it('does not set aria-current when not active', () => {
23-
render(<ul role="menu"><MenuItem href="/home">Home</MenuItem></ul>);
24-
expect(screen.getByRole('menuitem')).not.toHaveAttribute('aria-current');
23+
render(<ul><MenuItem href="/home">Home</MenuItem></ul>);
24+
expect(screen.getByRole('link', { name: 'Home' })).not.toHaveAttribute('aria-current');
2525
});
2626

27-
it('sets aria-disabled when disabled', () => {
28-
render(<ul role="menu"><MenuItem disabled>Locked</MenuItem></ul>);
29-
expect(screen.getByRole('menuitem')).toHaveAttribute('aria-disabled', 'true');
27+
it('sets aria-disabled when disabled (link)', () => {
28+
render(<ul><MenuItem href="/locked" disabled>Locked</MenuItem></ul>);
29+
expect(screen.getByRole('link', { name: 'Locked' })).toHaveAttribute('aria-disabled', 'true');
30+
});
31+
32+
it('disables the button natively when disabled', () => {
33+
render(<ul><MenuItem disabled>Locked</MenuItem></ul>);
34+
expect(screen.getByRole('button', { name: 'Locked' })).toBeDisabled();
3035
});
3136

3237
it('renders icon with aria-hidden', () => {
3338
const { container } = render(
34-
<ul role="menu"><MenuItem icon={<span></span>}>Starred</MenuItem></ul>,
39+
<ul><MenuItem icon={<span></span>}>Starred</MenuItem></ul>,
3540
);
3641
expect(container.querySelector('[aria-hidden="true"]')).toBeInTheDocument();
3742
});
3843

3944
it('renders children as label text', () => {
40-
render(<ul role="menu"><MenuItem href="/x">My Page</MenuItem></ul>);
45+
render(<ul><MenuItem href="/x">My Page</MenuItem></ul>);
4146
expect(screen.getByText('My Page')).toBeInTheDocument();
4247
});
4348
});

0 commit comments

Comments
 (0)