Skip to content

get ActionBar axe clean #4388

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 19, 2024
Merged

get ActionBar axe clean #4388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bd7d29b
get ActionBar axe clean
keithamus Mar 13, 2024
e9a8fb2
changeset
keithamus Mar 13, 2024
6e15212
fix type
keithamus Mar 14, 2024
7a8e239
fix type
keithamus Mar 14, 2024
8bdbb17
Merge branch 'main' into get-actionbar-axe-clean
keithamus Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/lemon-seahorses-argue.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

ActionBar now produces valid HTML
35 changes: 35 additions & 0 deletions packages/react/src/drafts/ActionBar/ActionBar.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import React from 'react'
import {behavesAsComponent} from '../../utils/testing'
import {render as HTMLRender} from '@testing-library/react'
import {axe} from 'jest-axe'

import ActionBar from './'
import {BoldIcon, CodeIcon, ItalicIcon, LinkIcon} from '@primer/octicons-react'

const SimpleActionBar = () => (
<ActionBar>
<ActionBar.IconButton icon={BoldIcon} aria-label="Default"></ActionBar.IconButton>
<ActionBar.IconButton icon={ItalicIcon} aria-label="Default"></ActionBar.IconButton>
<ActionBar.Divider />
<ActionBar.IconButton icon={CodeIcon} aria-label="Default"></ActionBar.IconButton>
<ActionBar.IconButton icon={LinkIcon} aria-label="Default"></ActionBar.IconButton>
</ActionBar>
)

describe('ActionBar', () => {
afterEach(() => {
jest.clearAllMocks()
})

behavesAsComponent({
Component: ActionBar,
options: {skipAs: true, skipSx: true},
toRender: () => <SimpleActionBar />,
})

it('should have no axe violations', async () => {
const {container} = HTMLRender(<SimpleActionBar />)
const results = await axe(container)
expect(results).toHaveNoViolations()
})
Comment on lines +30 to +34
Copy link
Member

Choose a reason for hiding this comment

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

Just wanted to share that axe is also available in the e2e tests but with the additional github rules if that's of interest! Wasn't sure how obvious it was.

})
8 changes: 4 additions & 4 deletions packages/react/src/drafts/ActionBar/ActionBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export type ActionBarProps = {

export type ActionBarIconButtonProps = IconButtonProps

const NavigationList = styled.ul`
const NavigationList = styled.div`
${sx};
`

Expand All @@ -54,7 +54,7 @@ const MoreMenuListItem = styled.li`
height: ${MORE_BTN_HEIGHT}px;
`

const ulStyles = {
const listStyles = {
display: 'flex',
listStyle: 'none',
whiteSpace: 'nowrap',
Expand Down Expand Up @@ -188,7 +188,7 @@ export const ActionBar: React.FC<React.PropsWithChildren<ActionBarProps>> = prop
}, [])

const navRef = useRef<HTMLDivElement>(null)
const listRef = useRef<HTMLUListElement>(null)
const listRef = useRef<HTMLDivElement>(null)
const moreMenuRef = useRef<HTMLLIElement>(null)
const moreMenuBtnRef = useRef<HTMLButtonElement>(null)
const containerRef = React.useRef<HTMLUListElement>(null)
Expand Down Expand Up @@ -254,7 +254,7 @@ export const ActionBar: React.FC<React.PropsWithChildren<ActionBarProps>> = prop
return (
<ActionBarContext.Provider value={{size, setChildrenWidth}}>
<Box ref={navRef} sx={getNavStyles()}>
<NavigationList sx={ulStyles} ref={listRef} role="list">
<NavigationList sx={listStyles} ref={listRef} role="toolbar">
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, toolbar makes total sense here. I tested the arrow focusing and it looks fine to me 👍

{listItems}
{menuItems.length > 0 && (
<MoreMenuListItem ref={moreMenuRef}>
Expand Down