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: ComboBoxのuseOptionsのロジックを整理する #5332

Merged
merged 12 commits into from
Feb 14, 2025
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { FaCaretDownIcon } from '../../Icon'
import { areComboBoxItemsEqual } from '../comboBoxHelper'
import { useFocusControl } from '../useFocusControl'
import { useListBox } from '../useListBox'
import { useOptions } from '../useOptions'
import { useMultiOptions } from '../useOptions'

import { MultiSelectedItem } from './MultiSelectedItem'
import { hasParentElementByClassName } from './multiComboBoxHelper'
Expand Down Expand Up @@ -176,7 +176,7 @@ const ActualMultiComboBox = <T,>(
const [uncontrolledInputValue, setUncontrolledInputValue] = useState('')
const inputValue = isInputControlled ? controlledInputValue : uncontrolledInputValue
const [isComposing, setIsComposing] = useState(false)
const { options } = useOptions({
const { options } = useMultiOptions({
items,
selected: selectedItems,
creatable,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { UnstyledButton } from '../../Button'
import { FaCaretDownIcon, FaCircleXmarkIcon } from '../../Icon'
import { Input } from '../../Input'
import { useListBox } from '../useListBox'
import { useOptions } from '../useOptions'
import { useSingleOptions } from '../useOptions'

import type { BaseProps, ComboBoxItem } from '../types'

Expand Down Expand Up @@ -154,7 +154,7 @@ const ActualSingleComboBox = <T,>(

useImperativeHandle<HTMLInputElement | null, HTMLInputElement | null>(ref, () => inputRef.current)

const { options } = useOptions({
const { options } = useSingleOptions({
items,
selected: selectedItem,
creatable,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { renderHook } from '@testing-library/react'
import React from 'react'

import { useOptions } from '../useOptions'
import { useMultiOptions } from '../useOptions'
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: useSingleOptionsのほうのテストって追加しなくても大丈夫そうですか? (これまでなさそうな雰囲気は理解しつつ、追加してもいいんじゃないかなと思って聞いています)

Copy link
Member Author

Choose a reason for hiding this comment

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

まー、内部処理に隠蔽されちゃってるところなんで、必要になったらでいい気がしてます


describe('useOptions', () => {
describe('useMultiOptions', () => {
describe('options', () => {
it('options が取得できること', () => {
const initialProps = {
Expand All @@ -15,7 +15,7 @@ describe('useOptions', () => {
selected: [{ label: 'label2', value: 'value2' }],
creatable: false,
}
const { result } = renderHook((props) => useOptions(props), { initialProps })
const { result } = renderHook((props) => useMultiOptions(props), { initialProps })
const options = result.current.options

expect(options.length).toBe(3)
Expand Down Expand Up @@ -43,7 +43,7 @@ describe('useOptions', () => {
creatable: false,
inputValue: '2',
}
const { result } = renderHook((props) => useOptions(props), { initialProps })
const { result } = renderHook((props) => useMultiOptions(props), { initialProps })
const options = result.current.options

expect(options.length).toBe(1)
Expand All @@ -57,7 +57,7 @@ describe('useOptions', () => {
creatable: true,
inputValue: 'input_data',
}
const { result, rerender } = renderHook((props) => useOptions(props), { initialProps })
const { result, rerender } = renderHook((props) => useMultiOptions(props), { initialProps })

const options1 = result.current.options
expect(options1.length).toBe(1)
Expand Down Expand Up @@ -96,7 +96,7 @@ describe('useOptions', () => {
creatable: false,
isItemSelected,
}
renderHook((props) => useOptions(props), { initialProps })
renderHook((props) => useMultiOptions(props), { initialProps })
expect(isItemSelected).toHaveBeenNthCalledWith(
1,
{ label: 'label1', value: 'value1' },
Expand Down Expand Up @@ -132,7 +132,7 @@ describe('useOptions', () => {
selected: [{ label: element, value: 'value3' }],
creatable: false,
}
const { result } = renderHook((props) => useOptions(props), { initialProps })
const { result } = renderHook((props) => useMultiOptions(props), { initialProps })
const options = result.current.options

expect(options.length).toBe(3)
Expand Down Expand Up @@ -165,7 +165,7 @@ describe('useOptions', () => {
creatable: false,
inputValue: 'label3',
}
const { result } = renderHook((props) => useOptions(props), { initialProps })
const { result } = renderHook((props) => useMultiOptions(props), { initialProps })
const options = result.current.options

expect(options.length).toBe(1)
Expand Down Expand Up @@ -199,7 +199,7 @@ describe('useOptions', () => {
selected: [{ label: labelElement3, value: 'value3' }],
creatable: false,
}
const { result } = renderHook((props) => useOptions(props), { initialProps })
const { result } = renderHook((props) => useMultiOptions(props), { initialProps })
const options = result.current.options

expect(options.length).toBe(3)
Expand Down Expand Up @@ -227,7 +227,7 @@ describe('useOptions', () => {
creatable: false,
inputValue: 'label3',
}
const { result } = renderHook((props) => useOptions(props), { initialProps })
const { result } = renderHook((props) => useMultiOptions(props), { initialProps })
const options = result.current.options

expect(options.length).toBe(1)
Expand All @@ -245,7 +245,7 @@ describe('useOptions', () => {
selected: [{ label: newLabelElement1, value: 'value1' }],
creatable: false,
}
const { result } = renderHook((props) => useOptions(props), { initialProps })
const { result } = renderHook((props) => useMultiOptions(props), { initialProps })
const options = result.current.options

expect(options.length).toBe(1)
Expand Down
113 changes: 64 additions & 49 deletions packages/smarthr-ui/src/components/ComboBox/useOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,71 +9,86 @@ const defaultIsItemSelected = <T>(
selectedItems: Array<ComboBoxItem<T>>,
) => selectedItems.some((item) => areComboBoxItemsEqual(item, targetItem))

export function useOptions<T>({
items,
selected,
creatable,
inputValue = '',
isFilteringDisabled = false,
isItemSelected = defaultIsItemSelected,
}: {
type BaseUseOptionsProps<T> = {
items: Array<ComboBoxItem<T>>
selected: (ComboBoxItem<T> | null) | Array<ComboBoxItem<T>>
creatable: boolean
inputValue?: string
isFilteringDisabled?: boolean
}

export const useSingleOptions = <T>({
selected,
...rest
}: BaseUseOptionsProps<T> & {
selected: ComboBoxItem<T> | null
}) => {
const isSelected = useCallback(
(item: ComboBoxItem<T>) => selected !== null && areComboBoxItemsEqual(selected, item),
[selected],
)

return useOptions<T>(rest, isSelected)
}

export const useMultiOptions = <T>({
selected,
isItemSelected = defaultIsItemSelected,
...rest
}: BaseUseOptionsProps<T> & {
selected: Array<ComboBoxItem<T>>
isItemSelected?: (targetItem: ComboBoxItem<T>, selectedItems: Array<ComboBoxItem<T>>) => boolean
}) {
const isInputValueAddable = useMemo(
() => creatable && inputValue !== '' && !items.some((item) => item.label === inputValue),
[creatable, inputValue, items],
}) => {
const isSelected = useCallback(
(item: ComboBoxItem<T>) => isItemSelected(item, selected),
[selected, isItemSelected],
)

return useOptions<T>(rest, isSelected)
}

function useOptions<T>(
{ items, creatable, inputValue = '', isFilteringDisabled = false }: BaseUseOptionsProps<T>,
isSelected: (item: ComboBoxItem<T>) => boolean,
) {
const newItemId = useId()
const optionIdPrefix = useId()
const getOptionId = useCallback(
(optionIndex: number) => `${optionIdPrefix}-${optionIndex}`,
[optionIdPrefix],
)
Comment on lines -34 to -37
Copy link
Member Author

Choose a reason for hiding this comment

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

メソッド化する意味が薄かったため削除しています


const isSelected = useCallback(
(item: ComboBoxItem<T>) => {
if (Array.isArray(selected)) {
return isItemSelected(item, selected)
} else {
return selected !== null && areComboBoxItemsEqual(selected, item)
}
},
[isItemSelected, selected],
const existedOptions: Array<ComboBoxOption<T>> = useMemo(
() =>
items.map((item, i) => ({
id: `${optionIdPrefix}-${i}`,
selected: isSelected(item),
isNew: false,
item,
})),
[isSelected, items, optionIdPrefix],
)
const addingOption: ComboBoxOption<T> | null = useMemo(
() =>
creatable && inputValue && items.every((item) => item.label !== inputValue)
? {
id: newItemId,
isNew: true,
selected: false,
item: { label: inputValue, value: inputValue },
}
: null,
[inputValue, items, creatable, newItemId],
Comment on lines +66 to +76
Copy link
Member Author

Choose a reason for hiding this comment

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

creatableがtrueになる可能性はかなり低く、nullになる可能性が高くなります。
別途メモ化することで他のmemoが更新される可能性が下がります

)

const allOptions: Array<ComboBoxOption<T>> = useMemo(() => {
const _options = items.map((item, i) => ({
id: getOptionId(i),
selected: isSelected(item),
isNew: false,
item,
}))
if (isInputValueAddable) {
const addingOption = {
id: newItemId,
isNew: true,
selected: false,
item: { label: inputValue, value: inputValue },
}
return [addingOption, ..._options]
}
return _options
}, [getOptionId, inputValue, isInputValueAddable, isSelected, items, newItemId])
const allOptions: Array<ComboBoxOption<T>> = useMemo(
() => (addingOption ? [addingOption, ...existedOptions] : existedOptions),
[existedOptions, addingOption],
)

const options = useMemo(() => {
if (isFilteringDisabled) {
if (isFilteringDisabled || !inputValue) {
return allOptions
}
return allOptions.filter(({ item: { label } }) => {
if (!inputValue) return true
Copy link
Member Author

Choose a reason for hiding this comment

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

filter内で処理する意味がないため、isFilteringDisabledと同時にチェックするようにしています

return convertMatchableString(innerText(label)).includes(convertMatchableString(inputValue))
})

return allOptions.filter(({ item: { label } }) =>
convertMatchableString(innerText(label)).includes(convertMatchableString(inputValue)),
)
}, [allOptions, inputValue, isFilteringDisabled])

return {
Expand Down