Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/fair-bars-smile.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': major
---

Support nested children in ActionBar.
30 changes: 30 additions & 0 deletions packages/react/src/ActionBar/ActionBar.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,33 @@ export const Default = () => (
<ActionBar.IconButton icon={TasklistIcon} aria-label="Task List"></ActionBar.IconButton>
</ActionBar>
)

const BoldButton = () => <ActionBar.IconButton icon={BoldIcon} aria-label="Bold"></ActionBar.IconButton>

const FormattingButtons = () => (
<>
<BoldButton />
<ActionBar.IconButton icon={ItalicIcon} aria-label="Italic"></ActionBar.IconButton>
<ActionBar.IconButton icon={CodeIcon} aria-label="Code"></ActionBar.IconButton>
<ActionBar.IconButton icon={LinkIcon} aria-label="Link"></ActionBar.IconButton>
</>
)

const AdvancedFormattingButtons = () => (
<>
<ActionBar.IconButton icon={FileAddedIcon} aria-label="File Added"></ActionBar.IconButton>
<ActionBar.IconButton icon={SearchIcon} aria-label="Search"></ActionBar.IconButton>
<ActionBar.IconButton icon={QuoteIcon} aria-label="Insert Quote"></ActionBar.IconButton>
<ActionBar.IconButton icon={ListUnorderedIcon} aria-label="Unordered List"></ActionBar.IconButton>
<ActionBar.IconButton icon={ListOrderedIcon} aria-label="Ordered List"></ActionBar.IconButton>
<ActionBar.IconButton icon={TasklistIcon} aria-label="Task List"></ActionBar.IconButton>
</>
)

export const DeepChildTree = () => (
<ActionBar aria-label="Toolbar">
<FormattingButtons />
<ActionBar.Divider />
<AdvancedFormattingButtons />
</ActionBar>
)
160 changes: 158 additions & 2 deletions packages/react/src/ActionBar/ActionBar.test.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import {describe, expect, it, afterEach, vi} from 'vitest'
import {render, screen, act} from '@testing-library/react'
import userEvent from '@testing-library/user-event'

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

describe('ActionBar', () => {
afterEach(() => {
Expand Down Expand Up @@ -78,3 +78,159 @@ describe('ActionBar', () => {
expect(onClick).toHaveBeenCalled()
})
})

describe('ActionBar Registry System', () => {
it('should preserve order with deep nesting', () => {
render(
<ActionBar aria-label="Deep test">
<div>
<ActionBar.IconButton icon={BoldIcon} aria-label="First" />
</div>
<ActionBar.IconButton icon={ItalicIcon} aria-label="Second" />
<div>
<ActionBar.IconButton icon={CodeIcon} aria-label="Third" />
</div>
</ActionBar>,
)

const buttons = screen.getAllByRole('button')
expect(buttons).toHaveLength(3)
expect(buttons[0]).toHaveAccessibleName('First')
expect(buttons[1]).toHaveAccessibleName('Second')
expect(buttons[2]).toHaveAccessibleName('Third')
})

it('should handle conditional rendering without breaking order', async () => {
const ConditionalTest = () => {
const [show, setShow] = useState([true, true, true])

return (
<div>
<ActionBar aria-label="Conditional">
{show[0] && <ActionBar.IconButton icon={BoldIcon} aria-label="First" />}
{show[1] && <ActionBar.IconButton icon={ItalicIcon} aria-label="Second" />}
{show[2] && <ActionBar.IconButton icon={CodeIcon} aria-label="Third" />}
</ActionBar>
<button type="button" onClick={() => setShow([false, true, true])}>
Hide first
</button>
<button type="button" onClick={() => setShow([true, true, true])}>
Show all
</button>
</div>
)
}

const user = userEvent.setup()
render(<ConditionalTest />)

// Initially should have 3 buttons
expect(screen.getAllByRole('button', {name: /First|Second|Third/})).toHaveLength(3)

// Hide first button
await user.click(screen.getByText('Hide first'))

const buttonsAfterHide = screen.getAllByRole('button', {name: /Second|Third/})
expect(buttonsAfterHide).toHaveLength(2)
expect(buttonsAfterHide[0]).toHaveAccessibleName('Second')
expect(buttonsAfterHide[1]).toHaveAccessibleName('Third')

// Show first button again
await user.click(screen.getByText('Show all'))

const buttonsAfterShow = screen.getAllByRole('button', {name: /First|Second|Third/})
expect(buttonsAfterShow).toHaveLength(3)
expect(buttonsAfterShow[0]).toHaveAccessibleName('First')
expect(buttonsAfterShow[1]).toHaveAccessibleName('Second')
expect(buttonsAfterShow[2]).toHaveAccessibleName('Third')
})

it('should handle fragments and array mapping', () => {
render(
<ActionBar aria-label="Fragment test">
<>
<ActionBar.IconButton icon={BoldIcon} aria-label="In fragment" />
{[1, 2].map(i => (
<ActionBar.IconButton key={i} icon={ItalicIcon} aria-label={`Mapped ${i}`} />
))}
</>
<ActionBar.IconButton icon={CodeIcon} aria-label="After fragment" />
</ActionBar>,
)

const buttons = screen.getAllByRole('button')
expect(buttons).toHaveLength(4)
expect(buttons[0]).toHaveAccessibleName('In fragment')
expect(buttons[1]).toHaveAccessibleName('Mapped 1')
expect(buttons[2]).toHaveAccessibleName('Mapped 2')
expect(buttons[3]).toHaveAccessibleName('After fragment')
})

it('should handle rapid re-renders without losing registry data', async () => {
const RapidRerenderTest = () => {
const [count, setCount] = useState(0)

return (
<div>
<ActionBar aria-label="Rapid rerender">
<ActionBar.IconButton icon={BoldIcon} aria-label={`Button ${count}`} />
</ActionBar>
<button type="button" onClick={() => setCount(c => c + 1)}>
Increment
</button>
</div>
)
}

const user = userEvent.setup()
render(<RapidRerenderTest />)

// Rapidly trigger re-renders
for (let i = 0; i < 10; i++) {
await user.click(screen.getByText('Increment'))
Comment on lines +188 to +190
Copy link
Preview

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

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

The test performs 10 sequential async clicks which could be slow and potentially flaky. Consider reducing the number of iterations or using a more efficient approach to test rapid re-renders.

Suggested change
// Rapidly trigger re-renders
for (let i = 0; i < 10; i++) {
await user.click(screen.getByText('Increment'))
// Rapidly trigger re-renders using act to batch updates
for (let i = 0; i < 10; i++) {
await act(async () => {
screen.getByText('Increment').click()
})

Copilot uses AI. Check for mistakes.

}

expect(screen.getByRole('button', {name: 'Button 10'})).toBeInTheDocument()
})

it('should handle zero-width scenarios gracefully', () => {
render(
<div style={{width: 0, overflow: 'hidden'}}>
<ActionBar aria-label="Zero width">
<ActionBar.IconButton icon={BoldIcon} aria-label="Zero width button" />
</ActionBar>
</div>,
)

// Component should still render even with zero width
expect(screen.getByRole('button', {name: 'Zero width button'})).toBeInTheDocument()
})

it('should clean up registry on unmount', async () => {
const UnmountTest = () => {
const [mounted, setMounted] = useState(true)

return (
<div>
{mounted && (
<ActionBar aria-label="Unmount test">
<ActionBar.IconButton icon={BoldIcon} aria-label="Will unmount" />
</ActionBar>
)}
<button type="button" onClick={() => setMounted(false)}>
Unmount
</button>
</div>
)
}

const user = userEvent.setup()
render(<UnmountTest />)

expect(screen.getByRole('button', {name: 'Will unmount'})).toBeInTheDocument()

await user.click(screen.getByText('Unmount'))

expect(screen.queryByRole('button', {name: 'Will unmount'})).not.toBeInTheDocument()
})
})
Loading
Loading