Skip to content
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.

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

chore: DropdownMenuButton の内部ロジックを整理する #5314

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
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
Original file line number Diff line number Diff line change
Expand Up @@ -70,59 +70,88 @@ export const DropdownMenuButton: FC<Props & ElementProps> = ({
label,
children,
triggerSize,
onlyIconTrigger = false,
triggerIcon: TriggerIcon,
onlyIconTrigger,
Copy link
Member Author

Choose a reason for hiding this comment

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

比較しかしていないため、初期値代入は不要でした

triggerIcon,
className,
...props
...rest
}) => {
const containerRef = React.useRef<HTMLUListElement>(null)

const triggerLabel = useMemo(() => {
const Icon = TriggerIcon || FaEllipsisIcon
return onlyIconTrigger ? (
<Icon alt={typeof label === 'string' ? label : innerText(label)} />
) : (
label
)
}, [label, TriggerIcon, onlyIconTrigger])
const triggerSuffix = useMemo(
() => (onlyIconTrigger ? undefined : <FaCaretDownIcon alt="候補を開く" />),
[onlyIconTrigger],
)

useKeyboardNavigation(containerRef)

const styles = useMemo(
() => ({
triggerWrapper: triggerWrapper({ className }),
triggerButton: triggerButton(),
actionList: actionList(),
}),
[className],
)

return (
<Dropdown>
<DropdownTrigger className={triggerWrapper({ className })}>
<Button
{...props}
suffix={triggerSuffix}
size={triggerSize}
square={onlyIconTrigger}
className={triggerButton()}
>
{triggerLabel}
</Button>
</DropdownTrigger>
<MemoizedTriggerButton
{...rest}
label={label}
onlyIconTrigger={onlyIconTrigger}
triggerIcon={triggerIcon}
triggerSize={triggerSize}
wrapperStyle={styles.triggerWrapper}
buttonStyle={styles.triggerButton}
/>
<DropdownContent>
<menu ref={containerRef} className={actionList()}>
<menu ref={containerRef} className={styles.actionList}>
{renderButtonList(children)}
</menu>
</DropdownContent>
</Dropdown>
)
}

const MemoizedTriggerButton = React.memo<
Pick<Props, 'onlyIconTrigger' | 'triggerSize' | 'label' | 'triggerIcon'> &
ElementProps & { wrapperStyle: string; buttonStyle: string }
>(({ onlyIconTrigger, triggerSize, label, triggerIcon, wrapperStyle, buttonStyle, ...rest }) => (
<DropdownTrigger className={wrapperStyle}>
<Button
{...rest}
suffix={<ButtonSuffixIcon onlyIconTrigger={onlyIconTrigger} />}
size={triggerSize}
square={onlyIconTrigger}
className={buttonStyle}
>
<TriggerLabelText label={label} onlyIconTrigger={onlyIconTrigger} triggerIcon={triggerIcon} />
</Button>
</DropdownTrigger>
))

const TriggerLabelText = React.memo<Pick<Props, 'label' | 'onlyIconTrigger' | 'triggerIcon'>>(
({ label, onlyIconTrigger, triggerIcon }) => {
if (!onlyIconTrigger) {
return label
}

const Icon = triggerIcon || FaEllipsisIcon

return <Icon alt={typeof label === 'string' ? label : innerText(label)} />
},
)

const ButtonSuffixIcon = React.memo<Pick<Props, 'onlyIconTrigger'>>(
({ onlyIconTrigger }) => !onlyIconTrigger && <FaCaretDownIcon alt="候補を開く" />,
)

export const renderButtonList = (children: Actions) =>
React.Children.map(children, (item): ReactNode => {
if (!(item && React.isValidElement(item))) return null
if (item.type === React.Fragment) {
return renderButtonList(item.props.children)
if (!item || !React.isValidElement(item)) {
return null
}

if (item.type === DropdownMenuGroup) {
return item
switch (item.type) {
case React.Fragment:
return renderButtonList(item.props.children)
case DropdownMenuGroup:
return item
}

const actualElement = React.cloneElement(item as ReactElement, {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { ComponentProps, type PropsWithChildren, type ReactNode } from 'react'
import React, { ComponentProps, type PropsWithChildren, type ReactNode, useMemo } from 'react'
import { tv } from 'tailwind-variants'

import { Text } from '../../Text'
Expand Down Expand Up @@ -36,13 +36,28 @@ export const DropdownMenuGroup: React.FC<Props & ElementProps> = ({
name,
children,
className,
}) => (
<li className={group({ className })}>
{name && (
<Text as="p" size="S" weight="bold" color="TEXT_GREY" leading="NONE" className={groupName()}>
{name}
}) => {
const styles = useMemo(
() => ({
group: group({ className }),
groupName: groupName(),
}),
[className],
)

return (
<li className={styles.group}>
<NameText className={styles.groupName}>{name}</NameText>
<ul>{renderButtonList(children)}</ul>
</li>
)
}

const NameText = React.memo<PropsWithChildren<{ className: string }>>(
({ children, className }) =>
children && (
<Text as="p" size="S" weight="bold" color="TEXT_GREY" leading="NONE" className={className}>
{children}
</Text>
Comment on lines +56 to +60
Copy link
Member Author

Choose a reason for hiding this comment

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

childrenに対してReactNodeが渡されますが、たいていはstringになるはずなのでmemo化しました

)}
<ul>{renderButtonList(children)}</ul>
</li>
),
)
Original file line number Diff line number Diff line change
Expand Up @@ -4,35 +4,63 @@ const matchesDisabledState = (element: Element): boolean =>
element.matches(':disabled') || element.getAttribute('aria-disabled') === 'true'

const isElementDisabled = (element: Element): boolean => {
if (matchesDisabledState(element)) return true
return Array.from(element.querySelectorAll('*')).some((child) => matchesDisabledState(child))
Copy link
Member Author

Choose a reason for hiding this comment

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

matchesDisabledState を呼び出すための無名関数を生成していましたが、引数が完全一致のため、無意味でした。
matchesDisabledStateを直接参照渡しするようにしました

if (matchesDisabledState(element)) {
return true
}

return Array.from(element.querySelectorAll('*')).some(matchesDisabledState)
}

const moveFocus = (
direction: number,
enabledItems: Element[],
focusedIndex: number,
hoveredItem: Element | null,
) => {
const calculateNextIndex = () => {
if (focusedIndex > -1) {
// フォーカスされているアイテムが存在する場合
return (focusedIndex + direction + enabledItems.length) % enabledItems.length
}
const KEY_UP_REGEX = /^(Arrow)?(Up|Left)$/
const KEY_DOWN_REGEX = /^(Arrow)?(Down|Right)$/

const moveFocus = (element: Element, direction: 1 | -1) => {
const { hoveredItem, tabbableItems, focusedIndex } = Array.from(
element.querySelectorAll('li > *'),
).reduce(
(
acc: {
hoveredItem: Element | null
tabbableItems: Element[]
focusedIndex: number
},
item,
) => {
if (item.matches(':hover') && acc.hoveredItem === null) {
acc.hoveredItem = item
}

if (hoveredItem) {
// ホバー状態のアイテムが存在する場合
return (
(enabledItems.indexOf(hoveredItem) + direction + enabledItems.length) % enabledItems.length
)
}
if (!isElementDisabled(item)) {
acc.tabbableItems.push(item)

if (document.activeElement === item) {
acc.focusedIndex = acc.tabbableItems.length - 1
}
}

// どちらもない場合は最初のアイテムからスタート
return direction > 0 ? 0 : enabledItems.length - 1
return acc
},
{
hoveredItem: null,
tabbableItems: [],
focusedIndex: -1,
},
)

let nextIndex = 0

if (focusedIndex > -1) {
// フォーカスされているアイテムが存在する場合
nextIndex = (focusedIndex + direction + tabbableItems.length) % tabbableItems.length
} else if (hoveredItem) {
// ホバー状態のアイテムが存在する場合
nextIndex =
(tabbableItems.indexOf(hoveredItem) + direction + tabbableItems.length) % tabbableItems.length
} else if (direction === -1) {
nextIndex = tabbableItems.length - 1
Comment on lines +50 to +60
Copy link
Member Author

Choose a reason for hiding this comment

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

元々は関数内にロジックが隠蔽されていましたが、変数の汚染などもないし、nextIndexに値を入れるだけだったのでただのif-elseにしました

}

const nextIndex = calculateNextIndex()
const nextItem = enabledItems[nextIndex]
const nextItem = tabbableItems[nextIndex]

if (nextItem instanceof HTMLElement) {
nextItem.focus()
Expand All @@ -46,57 +74,18 @@ const useKeyboardNavigation = (containerRef: React.RefObject<HTMLElement>) => {
return
}

const allItems = Array.from(containerRef.current.querySelectorAll('li > *'))
const {
hoveredItem,
tabbableItems: enabledItems,
focusedIndex,
} = allItems.reduce(
(
acc: {
hoveredItem: Element | null
tabbableItems: Element[]
focusedIndex: number
},
item,
) => {
if (item.matches(':hover') && acc.hoveredItem === null) {
acc.hoveredItem = item
}

const isDisabled = isElementDisabled(item)

if (isDisabled) {
return acc
}

acc.tabbableItems.push(item)
if (document.activeElement === item) {
acc.focusedIndex = acc.tabbableItems.length - 1
}

return acc
},
{
hoveredItem: null,
tabbableItems: [],
focusedIndex: -1,
},
)

if (['Up', 'ArrowUp', 'Left', 'ArrowLeft'].includes(e.key)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

配列に対してincludesを実行するより、正規表現一発でのチェックのほうが高速のため調整しています

moveFocus(-1, enabledItems, focusedIndex, hoveredItem)
}

if (['Down', 'ArrowDown', 'Right', 'ArrowRight'].includes(e.key)) {
moveFocus(1, enabledItems, focusedIndex, hoveredItem)
Comment on lines -87 to -92
Copy link
Member Author

Choose a reason for hiding this comment

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

直前で計算している allItems ですが、押したキーが合致しない場合、完全に無駄処理になります。
チェックする順序を調整して無駄処理が発生しないようにしています

if (KEY_UP_REGEX.test(e.key)) {
moveFocus(containerRef.current, -1)
} else if (KEY_DOWN_REGEX.test(e.key)) {
moveFocus(containerRef.current, 1)
}
},
[containerRef],
)

useEffect(() => {
document.addEventListener('keydown', handleKeyDown)

return () => {
document.removeEventListener('keydown', handleKeyDown)
}
Expand Down
Loading