Skip to content

Commit

Permalink
fix(MenuGroup): reduce rerenders when opening groups (#1399)
Browse files Browse the repository at this point in the history
  • Loading branch information
stipsan authored Jul 18, 2024
1 parent b830db1 commit 9c982ea
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 28 deletions.
34 changes: 17 additions & 17 deletions src/core/components/menu/menuGroup.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {ChevronRightIcon} from '@sanity/icons'
import {isValidElement, useCallback, useEffect, useRef, useState} from 'react'
import {isValidElement, useCallback, useEffect, useState} from 'react'
import {isValidElementType} from 'react-is'
import {useArrayProp} from '../../hooks'
import {Box, Flex, Popover, PopoverProps, Text} from '../../primitives'
Expand Down Expand Up @@ -58,7 +58,7 @@ export function MenuGroup(
} = menu
const [rootElement, setRootElement] = useState<HTMLButtonElement | HTMLDivElement | null>(null)
const [open, setOpen] = useState(false)
const shouldFocusRef = useRef<'first' | 'last' | null>(null)
const [shouldFocus, setShouldFocus] = useState<'first' | 'last' | null>(null)
const active = Boolean(activeElement) && activeElement === rootElement
const [withinMenu, setWithinMenu] = useState(false)

Expand Down Expand Up @@ -88,22 +88,17 @@ export function MenuGroup(

const handleClick = useCallback(
(event: React.MouseEvent<HTMLDivElement>) => {
if (onClick) onClick(event)

shouldFocusRef.current = 'first'
onClick?.(event)

setShouldFocus('first')
setOpen(true)

requestAnimationFrame(() => {
shouldFocusRef.current = null
})
},
[onClick],
)

const handleChildItemClick = useCallback(() => {
setOpen(false)
if (onItemClick) onItemClick()
onItemClick?.()
}, [onItemClick])

const handleMenuMouseEnter = useCallback(() => setWithinMenu(true), [])
Expand All @@ -121,6 +116,16 @@ export function MenuGroup(
if (!open) setWithinMenu(false)
}, [open])

// Reset the shouldFocus state after it has been used
useEffect(() => {
if (!shouldFocus) return
// The useMenuController effect that handles `shouldFocus` schedules a request animation frame where it's processed.
// By doing the same here, we ensure that the reset is processed after the focus change.
const rafId = requestAnimationFrame(() => setShouldFocus(null))

return () => cancelAnimationFrame(rafId)
}, [shouldFocus])

const childMenu = (
<Menu
onClickOutside={onClickOutside}
Expand All @@ -129,7 +134,7 @@ export function MenuGroup(
onKeyDown={handleMenuKeyDown}
onMouseEnter={handleMenuMouseEnter}
registerElement={registerElement}
shouldFocus={shouldFocusRef.current}
shouldFocus={shouldFocus}
>
{children}
</Menu>
Expand All @@ -143,15 +148,10 @@ export function MenuGroup(
}

if (event.key === 'ArrowRight') {
shouldFocusRef.current = 'first'

setShouldFocus('first')
setOpen(true)
setWithinMenu(true)

requestAnimationFrame(() => {
shouldFocusRef.current = null
})

return
}
}, [])
Expand Down
8 changes: 2 additions & 6 deletions src/core/components/menu/useMenuController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ export function useMenuController(props: {
useEffect(() => {
if (!mounted) return

const rafId = window.requestAnimationFrame(() => {
const rafId = requestAnimationFrame(() => {
if (activeIndex === -1) {
if (shouldFocus === 'first') {
const focusableElements = _getFocusableElements(elementsRef.current)
Expand All @@ -194,7 +194,6 @@ export function useMenuController(props: {
const currentIndex = elementsRef.current.indexOf(el)

setActiveIndex(currentIndex)
activeIndexRef.current = currentIndex
}
}

Expand All @@ -206,7 +205,6 @@ export function useMenuController(props: {
const currentIndex = elementsRef.current.indexOf(el)

setActiveIndex(currentIndex)
activeIndexRef.current = currentIndex
}
}

Expand All @@ -218,9 +216,7 @@ export function useMenuController(props: {
element?.focus()
})

return () => {
window.cancelAnimationFrame(rafId)
}
return () => cancelAnimationFrame(rafId)
}, [activeIndex, mounted, setActiveIndex, shouldFocus])

return {
Expand Down
2 changes: 1 addition & 1 deletion src/core/components/toast/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const loadingAnimation = keyframes`
}
100% {
width: 100%;
}
}
`

const LOADING_BAR_HEIGHT = 2
Expand Down
6 changes: 2 additions & 4 deletions src/core/hooks/useClickOutsideEvent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,13 @@ export function useClickOutsideEvent(
return
}

const resolvedBoundaryElement =
typeof boundaryElement === 'function' ? boundaryElement() : boundaryElement
const resolvedBoundaryElement = boundaryElement?.()

if (resolvedBoundaryElement && !resolvedBoundaryElement.contains(target)) {
return
}

const resolvedElements = Array.isArray(elementsArg) ? elementsArg : elementsArg()
const elements = resolvedElements.flat()
const elements = elementsArg().flat()

for (const el of elements) {
if (!el) continue
Expand Down

0 comments on commit 9c982ea

Please sign in to comment.