Skip to content

feat: add React Compiler support for ActionMenu #8056

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

Open
joshblack wants to merge 1 commit into
base: main
Choose a base branch
from
+51 −29
Open

feat: add React Compiler support for ActionMenu #8056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/react-compiler-action-menu.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

ActionMenu: Improve rendering performance with React Compiler support
1 change: 0 additions & 1 deletion packages/react/script/react-compiler.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ const files = glob
return path.join(PACKAGE_DIR, match)
})
const unsupportedPatterns = [
'src/ActionMenu/**/*.tsx',
'src/Autocomplete/**/*.tsx',
'src/AvatarStack/**/*.tsx',
'src/Banner/**/*.tsx',
Expand Down
74 changes: 46 additions & 28 deletions packages/react/src/ActionMenu/ActionMenu.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,12 @@ function ExampleWithTooltip(): JSX.Element {
)
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
function ExampleWithTooltipV2(actionMenuTrigger: React.ReactElement<any>): JSX.Element {
function ExampleWithTooltipV2({
actionMenuTrigger,
}: {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
actionMenuTrigger: React.ReactElement<any>
}): JSX.Element {
Comment on lines +87 to +90
return (
<BaseStyles>
<ActionMenu>
Expand All @@ -99,6 +103,8 @@ function ExampleWithTooltipV2(actionMenuTrigger: React.ReactElement<any>): JSX.E
}

function ExampleWithReplaceableAnchor(): JSX.Element {
'use no memo'

Comment on lines 105 to +107
const anchorRef = useRef<HTMLButtonElement>(null)
const [open, setOpen] = useState(false)
const [anchorKey, setAnchorKey] = useState(0)
Expand Down Expand Up @@ -392,11 +398,13 @@ describe('ActionMenu', () => {

it('should open menu on menu button click and it is wrapped with tooltip v2', async () => {
const component = HTMLRender(
ExampleWithTooltipV2(
<TooltipV2 text="Additional context about the menu button" direction="s">
<ActionMenu.Button>Toggle Menu</ActionMenu.Button>
</TooltipV2>,
),
<ExampleWithTooltipV2
actionMenuTrigger={
<TooltipV2 text="Additional context about the menu button" direction="s">
<ActionMenu.Button>Toggle Menu</ActionMenu.Button>
</TooltipV2>
}
/>,
)
const button = component.getByRole('button')

Expand All @@ -415,11 +423,13 @@ describe('ActionMenu', () => {

it('should display tooltip v2 when menu button is focused', async () => {
const component = HTMLRender(
ExampleWithTooltipV2(
<TooltipV2 text="Additional context about the menu button" direction="s">
<ActionMenu.Button>Toggle Menu</ActionMenu.Button>
</TooltipV2>,
),
<ExampleWithTooltipV2
actionMenuTrigger={
<TooltipV2 text="Additional context about the menu button" direction="s">
<ActionMenu.Button>Toggle Menu</ActionMenu.Button>
</TooltipV2>
}
/>,
)
const button = component.getByRole('button')
act(() => {
Expand All @@ -431,13 +441,15 @@ describe('ActionMenu', () => {

it('should open menu on menu anchor click and it is wrapped with tooltip v2', async () => {
const component = HTMLRender(
ExampleWithTooltipV2(
<ActionMenu.Anchor>
<TooltipV2 text="Additional context about the menu button" direction="n">
<Button>Toggle Menu</Button>
</TooltipV2>
</ActionMenu.Anchor>,
),
<ExampleWithTooltipV2
actionMenuTrigger={
<ActionMenu.Anchor>
<TooltipV2 text="Additional context about the menu button" direction="n">
<Button>Toggle Menu</Button>
</TooltipV2>
</ActionMenu.Anchor>
}
/>,
)
const button = component.getByRole('button')

Expand All @@ -449,13 +461,15 @@ describe('ActionMenu', () => {

it('should display tooltip v2 and menu anchor is focused', async () => {
const component = HTMLRender(
ExampleWithTooltipV2(
<ActionMenu.Anchor>
<TooltipV2 text="Additional context about the menu button" direction="n">
<Button>Toggle Menu</Button>
</TooltipV2>
</ActionMenu.Anchor>,
),
<ExampleWithTooltipV2
actionMenuTrigger={
<ActionMenu.Anchor>
<TooltipV2 text="Additional context about the menu button" direction="n">
<Button>Toggle Menu</Button>
</TooltipV2>
</ActionMenu.Anchor>
}
/>,
)
const button = component.getByRole('button')
act(() => {
Expand Down Expand Up @@ -805,11 +819,15 @@ describe('ActionMenu', () => {

const newAnchor = component.getByRole('button', {name: 'Open menu'})
expect(newAnchor).not.toBe(initialAnchor)
const newOverlay = component.baseElement.querySelector('[data-component="ActionMenu.Overlay"]') as HTMLElement
expect(newOverlay).not.toBeNull()

// The new anchor should have the same anchor-name re-applied, and the
// overlay should still reference it via position-anchor.
expect(newAnchor.style.getPropertyValue('anchor-name')).toBe(initialAnchorName)
expect(overlay.style.getPropertyValue('position-anchor')).toBe(initialPositionAnchor)
await waitFor(() => {
expect(newAnchor.style.getPropertyValue('anchor-name')).toBe(initialAnchorName)
expect(newOverlay.style.getPropertyValue('position-anchor')).toBe(initialPositionAnchor)
})
})
})

Expand Down
Loading