feat!: Combobox にフォーカスした時に検索できることが伝わる振る舞いに変更 #5790
Conversation
commit: |
uknmr
force-pushed
the
improve-Combobox-style
branch
from
7212c76 to
261c70c
Compare
これって |
| if (e.key === 'Enter' && !isFocused) { | ||
| // パネルが閉じている状態でEnterを押した場合、パネルを開く | ||
| e.preventDefault() | ||
| focus() | ||
| } else if (e.key === 'ArrowDown' && !isFocused) { | ||
| // パネルが閉じている状態で下矢印を押した場合、パネルを開く | ||
| e.preventDefault() | ||
| focus() | ||
| } |
There was a problem hiding this comment.
!isFocusedが何度も検証されていますし、if-else内の処理が完全に同一なのでまとめたいです。
// パネルが閉じている状態でEnter or 下矢印を押した場合、パネルを開く
if (!isFocused && /^(Enter|ArrowDown)$/.test(e.key)) {
e.preventDefault()
focus()
}
| searchValue, | ||
| onSearchValueChange: setSearchValue, | ||
| onChangeInput, | ||
| onEscape: () => setIsFocused(false), |
There was a problem hiding this comment.
今のところ、useCallbackする方針なのでやってもらえると 🙏
(再レンダリングの問題が起きる可能性は少ないですが、後々置きた場合の調査コストを考えるとやってあるほうが楽なので... 🙇 )
| if (relatedTarget) { | ||
| const comparison = currentTarget.compareDocumentPosition(relatedTarget) | ||
|
|
||
| const highlightedRef = useRef(highlighted) | ||
| // 順方向の場合のみ、パネルを開く | ||
| // DOCUMENT_POSITION_PRECEDING (2) = 前の要素が現在の要素より前にある | ||
| if (comparison & Node.DOCUMENT_POSITION_PRECEDING) { | ||
| focus() | ||
| } | ||
| } else { | ||
| // マウスクリックまたは初回フォーカスの場合は開く | ||
| focus() | ||
| } |
There was a problem hiding this comment.
if-elseのネストをせず1階層、かつ一つのifで処理をまとめられるので調整したいです。
if (
// マウスクリックまたは初回フォーカスの場合は開く
!relatedTarget ||
// 順方向の場合のみ、パネルを開く
// DOCUMENT_POSITION_PRECEDING (2) = 前の要素が現在の要素より前にある
(currentTarget.compareDocumentPosition(relatedTarget) & Node.DOCUMENT_POSITION_PRECEDING)
) {
focus()
}
| setInputValueIfUncontrolled('') | ||
| const handleClick = useMemo(() => { | ||
| if (disabled) { | ||
| return |
There was a problem hiding this comment.
好みの問題もありますが、このreturnは早期returnというよりは onClickで何も処理させない ための早期returnなのでundefinedであることに意味がありそうです。
なので return undefinedしてもらえると他開発者が読むときに理解しやすそう
| }, [disabled, focus]) | ||
| const actualOnChangeInput = useMemo(() => { | ||
| const handlers = [onChange, onChangeInput].filter((h) => !!h) | ||
| const handlers = [onChange].filter((h) => !!h) |
There was a problem hiding this comment.
onChangeInputがなくなり、配列である必要性がなくなっています。
関連ロジックを見直したほうが良さそう
Qs-F
left a comment
Qs-F
left a comment
There was a problem hiding this comment.
これって どんな内容で検索すればいいか？ などのヘルプとして使うものという認識だったんですが、本当に消して大丈夫です？
例えば社員番号、従業員名、部署が1アイテムに詰まっている場合 社員番号、氏名、部署で検索できます っていうヘルプメッセージを出している、みたいな使い方をしているプロダクトをいくつか知っています
これは本来SearchInputの役割なので、dropdownHelpMessageの役割ではない認識です！
なので検索対象をSearchInputのTooltipに渡せるようにしたいです！
関連URL
概要
Combobox が検索できることを伝えるために、Combobox の見た目としての Input と検索用の SearchInput を分けました。
変更内容
やってないこと
確認方法