diff --git a/.changeset/breezy-cars-hear.md b/.changeset/breezy-cars-hear.md new file mode 100644 index 00000000000..5d665c3080a --- /dev/null +++ b/.changeset/breezy-cars-hear.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +UnderlineNav2: Prevent item width calculation when they are null diff --git a/src/UnderlineNav2/UnderlineNav.test.tsx b/src/UnderlineNav2/UnderlineNav.test.tsx index ff303fdb070..7a419cadeea 100644 --- a/src/UnderlineNav2/UnderlineNav.test.tsx +++ b/src/UnderlineNav2/UnderlineNav.test.tsx @@ -1,6 +1,7 @@ import React from 'react' import '@testing-library/jest-dom/extend-expect' -import {fireEvent, render} from '@testing-library/react' +import {render} from '@testing-library/react' +import userEvent from '@testing-library/user-event' import { IconProps, CodeIcon, @@ -13,6 +14,7 @@ import { } from '@primer/octicons-react' import {UnderlineNav} from '.' +import {behavesAsComponent, checkExports, checkStoriesForAxeViolations} from '../utils/testing' // window.matchMedia() is not implemented by JSDOM so we have to create a mock: // https://jestjs.io/docs/manual-mocks#mocking-methods-which-are-not-implemented-in-jsdom @@ -32,10 +34,12 @@ Object.defineProperty(window, 'matchMedia', { const ResponsiveUnderlineNav = ({ selectedItemText = 'Code', - loadingCounters = false + loadingCounters = false, + displayExtraEl = false }: { selectedItemText?: string loadingCounters?: boolean + displayExtraEl?: boolean }) => { const items: {navigation: string; icon?: React.FC; counter?: number}[] = [ {navigation: 'Code', icon: CodeIcon}, @@ -48,74 +52,125 @@ const ResponsiveUnderlineNav = ({ {navigation: 'Settings', counter: 10}, {navigation: 'Security', icon: ShieldLockIcon} ] + return ( - - {items.map(item => ( - - {item.navigation} - - ))} - +
+ + {items.map(item => ( + + {item.navigation} + + ))} + + {displayExtraEl && } +
) } describe('UnderlineNav', () => { - it('renders aria-current attribute to be pages when an item is selected', () => { - const {getByText} = render() - const selectedNavLink = getByText('Code').closest('a') + behavesAsComponent({ + Component: UnderlineNav, + options: {skipAs: true, skipSx: true}, + toRender: () => + }) - expect(selectedNavLink?.getAttribute('aria-current')).toBe('page') + checkExports('UnderlineNav2', { + default: undefined, + UnderlineNav + }) + it('renders aria-current attribute to be pages when an item is selected', () => { + const {getByRole} = render() + const selectedNavLink = getByRole('link', {name: 'Code'}) + expect(selectedNavLink.getAttribute('aria-current')).toBe('page') }) it('renders aria-label attribute correctly', () => { - const {container} = render() + const {container, getByRole} = render() expect(container.getElementsByTagName('nav').length).toEqual(1) - const nav = container.getElementsByTagName('nav')[0] - + const nav = getByRole('navigation') expect(nav.getAttribute('aria-label')).toBe('Repository') }) it('renders icons correctly', () => { - const {container} = render() - const nav = container.getElementsByTagName('nav')[0] + const {getByRole} = render() + const nav = getByRole('navigation') expect(nav.getElementsByTagName('svg').length).toEqual(7) }) - it('fires onSelect on click and keypress', async () => { + it('fires onSelect on click', async () => { const onSelect = jest.fn() - const {getByText} = render( + const {getByRole} = render( Item 1 Item 2 Item 3 ) - const item = getByText('Item 1') - fireEvent.click(item) + const item = getByRole('link', {name: 'Item 1'}) + const user = userEvent.setup() + await user.click(item) expect(onSelect).toHaveBeenCalledTimes(1) - fireEvent.keyPress(item, {key: 'Enter', code: 13, charCode: 13}) + }) + it('fires onSelect on keypress', async () => { + const onSelect = jest.fn() + const {getByRole} = render( + + Item 1 + Item 2 + + Item 3 + + + ) + const item = getByRole('link', {name: 'Item 1'}) + const user = userEvent.setup() + await user.tab() // tab into the story, this should focus on the first link + expect(item).toEqual(document.activeElement) + await user.keyboard('{Enter}') + // Enter keypress fires both click and keypress events expect(onSelect).toHaveBeenCalledTimes(2) + await user.keyboard(' ') // space + expect(onSelect).toHaveBeenCalledTimes(3) }) it('respects counter prop', () => { - const {getByText} = render() - const item = getByText('Issues').closest('a') - const counter = item?.getElementsByTagName('span')[3] - expect(counter?.className).toContain('CounterLabel') - expect(counter?.textContent).toBe('120') + const {getByRole} = render() + const item = getByRole('link', {name: 'Issues 120'}) + const counter = item.getElementsByTagName('span')[3] + expect(counter.className).toContain('CounterLabel') + expect(counter.textContent).toBe('120') }) it('respects loadingCounters prop', () => { - const {getByText} = render() - const item = getByText('Actions').closest('a') - const loadingCounter = item?.getElementsByTagName('span')[2] - expect(loadingCounter?.className).toContain('LoadingCounter') - expect(loadingCounter?.textContent).toBe('') + const {getByRole} = render() + const item = getByRole('link', {name: 'Actions'}) + const loadingCounter = item.getElementsByTagName('span')[2] + expect(loadingCounter.className).toContain('LoadingCounter') + expect(loadingCounter.textContent).toBe('') }) it('renders a visually hidden h2 heading for screen readers when aria-label is present', () => { - const {container} = render() - const heading = container.getElementsByTagName('h2')[0] + const {getByRole} = render() + const heading = getByRole('heading', {name: 'Repository navigation'}) + // check if heading is h2 tag + expect(heading.tagName).toBe('H2') expect(heading.className).toContain('VisuallyHidden') expect(heading.textContent).toBe('Repository navigation') }) }) + +describe('Keyboard Navigation', () => { + it('should move focus to the next/previous item on the list with the tab key', async () => { + const {getByRole} = render() + const item = getByRole('link', {name: 'Code'}) + const nextItem = getByRole('link', {name: 'Issues 120'}) + const user = userEvent.setup() + await user.tab() // tab into the story, this should focus on the first link + expect(item).toEqual(document.activeElement) // check if the first item is focused + await user.tab() + // focus should be on the next item + expect(nextItem).toHaveFocus() + expect(nextItem.getAttribute('tabindex')).toBe('0') + }) +}) + +checkStoriesForAxeViolations('examples', '../UnderlineNav2/') diff --git a/src/UnderlineNav2/UnderlineNav.tsx b/src/UnderlineNav2/UnderlineNav.tsx index 9373d48925a..9a42de9f063 100644 --- a/src/UnderlineNav2/UnderlineNav.tsx +++ b/src/UnderlineNav2/UnderlineNav.tsx @@ -373,3 +373,5 @@ export const UnderlineNav = forwardRef( ) } ) + +UnderlineNav.displayName = 'UnderlineNav' diff --git a/src/UnderlineNav2/UnderlineNavItem.tsx b/src/UnderlineNav2/UnderlineNavItem.tsx index be89953c72a..22e6fa6bf03 100644 --- a/src/UnderlineNav2/UnderlineNavItem.tsx +++ b/src/UnderlineNav2/UnderlineNavItem.tsx @@ -79,32 +79,34 @@ export const UnderlineNavItem = forwardRef( } = useContext(UnderlineNavContext) useLayoutEffect(() => { - const domRect = (ref as MutableRefObject).current.getBoundingClientRect() + if (ref.current) { + const domRect = (ref as MutableRefObject).current.getBoundingClientRect() - const icon = Array.from((ref as MutableRefObject).current.children[0].children).find( - child => child.getAttribute('data-component') === 'icon' - ) + const icon = Array.from((ref as MutableRefObject).current.children[0].children).find( + child => child.getAttribute('data-component') === 'icon' + ) - const content = Array.from((ref as MutableRefObject).current.children[0].children).find( - child => child.getAttribute('data-component') === 'text' - ) as HTMLElement - const text = content.textContent as string + const content = Array.from((ref as MutableRefObject).current.children[0].children).find( + child => child.getAttribute('data-component') === 'text' + ) as HTMLElement + const text = content.textContent as string - const iconWidthWithMargin = icon - ? icon.getBoundingClientRect().width + - Number(getComputedStyle(icon).marginRight.slice(0, -2)) + - Number(getComputedStyle(icon).marginLeft.slice(0, -2)) - : 0 + const iconWidthWithMargin = icon + ? icon.getBoundingClientRect().width + + Number(getComputedStyle(icon).marginRight.slice(0, -2)) + + Number(getComputedStyle(icon).marginLeft.slice(0, -2)) + : 0 - setChildrenWidth({text, width: domRect.width}) - setNoIconChildrenWidth({text, width: domRect.width - iconWidthWithMargin}) - preSelected && selectedLink === undefined && setSelectedLink(ref as RefObject) + setChildrenWidth({text, width: domRect.width}) + setNoIconChildrenWidth({text, width: domRect.width - iconWidthWithMargin}) + preSelected && selectedLink === undefined && setSelectedLink(ref as RefObject) - // Only runs when a menu item is selected (swapping the menu item with the list item to keep it visible) - if (selectedLinkText === text) { - setSelectedLink(ref as RefObject) - if (typeof onSelect === 'function' && selectEvent !== null) onSelect(selectEvent) - setSelectedLinkText('') + // Only runs when a menu item is selected (swapping the menu item with the list item to keep it visible) + if (selectedLinkText === text) { + setSelectedLink(ref as RefObject) + if (typeof onSelect === 'function' && selectEvent !== null) onSelect(selectEvent) + setSelectedLinkText('') + } } }, [ ref, @@ -185,3 +187,5 @@ export const UnderlineNavItem = forwardRef( ) } ) as PolymorphicForwardRefComponent<'a', UnderlineNavItemProps> + +UnderlineNavItem.displayName = 'UnderlineNavItem' diff --git a/src/UnderlineNav2/__snapshots__/UnderlineNav.test.tsx.snap b/src/UnderlineNav2/__snapshots__/UnderlineNav.test.tsx.snap new file mode 100644 index 00000000000..a31b5ea8a09 --- /dev/null +++ b/src/UnderlineNav2/__snapshots__/UnderlineNav.test.tsx.snap @@ -0,0 +1,673 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`UnderlineNav renders consistently 1`] = ` +.c1 { + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; + padding-left: 16px; + padding-right: 16px; + -webkit-box-pack: start; + -webkit-justify-content: flex-start; + -ms-flex-pack: start; + justify-content: flex-start; + border-bottom: 1px solid; + border-bottom-color: hsla(210,18%,87%,1); + align: row; + -webkit-align-items: center; + -webkit-box-align: center; + -ms-flex-align: center; + align-items: center; +} + +.c3 { + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; + -webkit-flex-direction: column; + -ms-flex-direction: column; + flex-direction: column; + -webkit-align-items: center; + -webkit-box-align: center; + -ms-flex-align: center; + align-items: center; +} + +.c4 { + position: relative; + display: -webkit-inline-box; + display: -webkit-inline-flex; + display: -ms-inline-flexbox; + display: inline-flex; + color: #24292f; + text-align: center; + -webkit-text-decoration: none; + text-decoration: none; + padding-top: 8px; + padding-bottom: 8px; + font-size: 14px; +} + +.c4:focus { + outline: 0; +} + +.c4:focus > div[data-component="wrapper"] { + box-shadow: inset 0 0 0 2px #0969da; +} + +.c4:focus:not(:focus-visible) > div[data-component="wrapper"] { + box-shadow: none; +} + +.c4:focus-visible > div[data-component="wrapper"] { + box-shadow: inset 0 0 0 2px #0969da; +} + +.c4 span[data-content]::before { + content: attr(data-content); + display: block; + height: 0; + font-weight: 600; + visibility: hidden; + white-space: nowrap; +} + +.c4::after { + position: absolute; + right: 50%; + bottom: 0; + width: 100%; + height: 2px; + content: ""; + background-color: transparent; + border-radius: 0; + -webkit-transform: translate(50%,-50%); + -ms-transform: translate(50%,-50%); + transform: translate(50%,-50%); +} + +.c5 { + display: -webkit-inline-box; + display: -webkit-inline-flex; + display: -ms-inline-flexbox; + display: inline-flex; + padding-top: 4px; + padding-bottom: 4px; + padding-left: 8px; + padding-right: 8px; + border-radius: 6px; +} + +.c6 { + -webkit-align-items: center; + -webkit-box-align: center; + -ms-flex-align: center; + align-items: center; + display: -webkit-inline-box; + display: -webkit-inline-flex; + display: -ms-inline-flexbox; + display: inline-flex; + margin-right: 8px; +} + +.c7 { + margin-left: 8px; + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; + -webkit-align-items: center; + -webkit-box-align: center; + -ms-flex-align: center; + align-items: center; +} + +.c8 { + display: inline-block; + padding: 2px 5px; + font-size: 12px; + font-weight: 600; + line-height: 1; + border-radius: 20px; + color: #24292f; + background-color: rgba(175,184,193,0.2); +} + +.c8:empty { + display: none; +} + +.c0 { + position: absolute; + width: 1px; + height: 1px; + padding: 0; + margin: -1px; + overflow: hidden; + -webkit-clip: rect(0,0,0,0); + clip: rect(0,0,0,0); + white-space: nowrap; + border-width: 0; +} + +.c2 { + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; + list-style: none; + white-space: nowrap; + padding-top: 0; + padding-bottom: 0; + padding-left: 0; + padding-right: 0; + margin: 0; + margin-bottom: -1px; + -webkit-align-items: center; + -webkit-box-align: center; + -ms-flex-align: center; + align-items: center; + gap: 8px; + position: relative; +} + +@media (hover:hover) { + .c4:hover > div[data-component="wrapper"] { + background-color: rgba(175,184,193,0.2); + -webkit-transition: background .12s ease-out; + transition: background .12s ease-out; + } +} + +@media (forced-colors:active) { + .c4::after { + background-color: transparent; + } +} + + +`;