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

Use more semantic elements in SubNav #711

Merged
merged 13 commits into from
Aug 29, 2024
5 changes: 5 additions & 0 deletions .changeset/empty-poems-sort.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react-brand': patch
---

Fixed an issue where `SubNav` links and submenus didn't use sematically correct elements.
joshfarrant marked this conversation as resolved.
Show resolved Hide resolved
11 changes: 10 additions & 1 deletion packages/react/src/SubNav/SubNav.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@
display: contents;
}

.SubNav__links-overlay {
list-style: none;
margin: 0;
padding: 0;
}

/*
* Narrow breakpoint
*/
Expand Down Expand Up @@ -149,6 +155,9 @@
}

.SubNav__sub-menu {
list-style: none;
padding: 0;
margin: 0;
padding-inline-start: var(--base-size-16);
}
}
Expand Down Expand Up @@ -232,7 +241,7 @@
.SubNav__link-label::after {
content: '';
position: absolute;
bottom: -6px;
bottom: -9px;
joshfarrant marked this conversation as resolved.
Show resolved Hide resolved
left: 0;
width: 100%;
height: 2px;
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/SubNav/SubNav.module.css.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ declare const styles: {
readonly "SubNav__action-container": string;
readonly "SubNav__sub-menu-icon": string;
readonly "SubNav__sub-menu-children": string;
readonly "SubNav__links-overlay": string;
readonly "SubNav--open": string;
readonly "fade-in": string;
readonly "SubNav__links-overlay": string;
readonly "SubNav__links-overlay--open": string;
readonly "SubNav__overlay-toggle": string;
readonly "SubNav__link": string;
Expand Down
46 changes: 26 additions & 20 deletions packages/react/src/SubNav/SubNav.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export type SubNavProps = {

const _SubNavRoot = memo(({id, children, className, 'data-testid': testId, fullWidth, hasShadow}: SubNavProps) => {
const navRef = React.useRef<HTMLElement>(null)
const overlayRef = React.useRef<HTMLDivElement>(null)
const overlayRef = React.useRef<HTMLUListElement>(null)
const [isOpenAtNarrow, setIsOpenAtNarrow] = useState(false)
const idForLinkContainer = useId()

Expand Down Expand Up @@ -124,15 +124,15 @@ const _SubNavRoot = memo(({id, children, className, 'data-testid': testId, fullW
>
{HeadingChild && <div className={styles['SubNav__heading-container']}>{HeadingChild}</div>}
{LinkChildren.length && (
<div
<ul
ref={overlayRef}
id={idForLinkContainer}
className={clsx(styles['SubNav__links-overlay'], isOpenAtNarrow && styles['SubNav__links-overlay--open'])}
data-testid={testIds.overlay}
>
{LinkChildren}
{ActionChild && <div className={styles['SubNav__action-container']}>{ActionChild}</div>}
</div>
{ActionChild && <li className={styles['SubNav__action-container']}>{ActionChild}</li>}
</ul>
)}
<button
className={styles['SubNav__overlay-toggle']}
Expand Down Expand Up @@ -254,32 +254,38 @@ const SubNavLink = forwardRef<HTMLAnchorElement | HTMLDivElement, SubNavLinkProp
})

if (hasSubMenu) {
return <SubNavLinkWithSubmenu {...props} ref={ref as RefObject<HTMLDivElement>} />
return (
<li>
<SubNavLinkWithSubmenu {...props} ref={ref as RefObject<HTMLDivElement>} />
</li>
)
}

const {children, href, 'aria-current': ariaCurrent, 'data-testid': testId, className, ...rest} = props

return (
<a
href={href}
className={clsx(styles['SubNav__link'], ariaCurrent && styles['SubNav__link--active'], className)}
aria-current={ariaCurrent}
data-testid={testId || testIds.link}
ref={ref as RefObject<HTMLAnchorElement>}
{...rest}
>
<Text as="span" size="200" className={styles['SubNav__link-label']}>
{children}
</Text>
</a>
<li>
<a
href={href}
className={clsx(styles['SubNav__link'], ariaCurrent && styles['SubNav__link--active'], className)}
aria-current={ariaCurrent}
data-testid={testId || testIds.link}
ref={ref as RefObject<HTMLAnchorElement>}
{...rest}
>
<Text as="span" size="200" className={styles['SubNav__link-label']}>
{children}
</Text>
</a>
</li>
)
})

function _SubMenu({children, className, ...props}: PropsWithChildren<BaseProps<HTMLDivElement>>) {
function _SubMenu({children, className, ...props}: PropsWithChildren<BaseProps<HTMLUListElement>>) {
return (
<div className={clsx(styles['SubNav__sub-menu'], className)} {...props}>
<ul className={clsx(styles['SubNav__sub-menu'], className)} {...props}>
{children}
</div>
</ul>
)
}

Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
joshfarrant marked this conversation as resolved.
Show resolved Hide resolved
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
joshfarrant marked this conversation as resolved.
Show resolved Hide resolved
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading