chore: Dropdownコンポーネント内のロジックを整理する #5304
Conversation
| tabbablesInContent.some((inner) => { | ||
| for (const inner of tabbable(wrapperRef.current)) { |
There was a problem hiding this comment.
元々のコードではsomeでループされていましたが、副作用の有るコードであり、returnされる値は利用されていませんでした。
また全ての要素を捜査する必要があるわけではなく 特定の値が存在すれば、関数を呼び出す という意図の処理でした。
これらから 特定のアイテムが見つかれば実行する ことをわかりやすくし、処理自体を早期終了できるよう、for ofに変更しています
(tabbablesInContent.find(....) でも意図が伝えやすそうですが、for of とbreakのほうが処理早いしまぁ... 位の気持ちで採用しています。)
| } | ||
|
|
||
| if (e.target && e.target === dummyFocusRef.current) { | ||
| const trigger = getFirstTabbable(triggerElementRef) |
There was a problem hiding this comment.
ifの順序が変更されていますが、どちらもonClickCloserを呼び出すだけなので結果の変更はありません。
またgetFirstTabbableはロジックが別ファイルに隠蔽されているため、呼び出し順を変えることで多少の効率化が図れます
| const triggers = tabbable(triggerElementRef.current) | ||
| const trigger = triggers[triggers.length - 1] | ||
|
|
||
| const trigger = tabbable(triggerElementRef.current).at(-1) |
There was a problem hiding this comment.
実は 配列を一度変数化し、xxx.lengthを参照する方法よりatで指定するほうが、ブラウザにもよりますが大体5倍位速度が違います
(それでも十分高速ですが)
atのほうが読みやすいこともあり、修正しました。
一応atを使った場合のデメリットとして、3年以上前のブラウザは対応していない場合がある、というものがありますが、対応範囲外のため強気に修正しています
| onClickCloser() | ||
| return | ||
| } | ||
| } else if (KEY_ESCAPE.test(e.key)) { |
There was a problem hiding this comment.
複数回の比較を行うより、正規表現一回のほうが高速の場合が多いため置き換えています
| if (!parent) { | ||
| return false | ||
| } | ||
|
|
||
| const path = e.composedPath() | ||
| if (path.length === 0 || !parent) return false | ||
|
|
||
| if (path.length === 0) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
e.composedPath() の呼び出しは不要な場合がありえるため、ifを分割しました
| {includeDisabledTrigger(children) | ||
| ? children | ||
| : React.Children.map(children, (child) => { |
There was a problem hiding this comment.
このmap自体がonClickに追加処理を差し込むためのものですが、そもそも子がdisabledな場合はその追加処理自体が不要になるため、map外でチェックするようにロジックを修正しました
| triggers.forEach((trigger) => { | ||
| trigger.setAttribute('aria-expanded', String(active)) | ||
| trigger.setAttribute('aria-expanded', active.toString()) |
There was a problem hiding this comment.
Stringは内部的にtoStringを呼び出すため、基本的にtoStringを呼び出すほうが文字列変換ロジックとしては高速です。
またStringは
- Symbolを変換できる唯一のビルトインの文字列変換ロジックであること
- そのためここでSymbolが渡されてくる可能性があるのか？などの思考が派生する可能性がある
- new Stringで処理すると歴史的経緯で大変バグりやすく、一瞬考えてしまう
- newをつけるとObjectになってしまう
など、混乱を生む可能性があるのでtoStringに置き換えました。
| className: contentInner({ isActive, className }), | ||
| style: { |
There was a problem hiding this comment.
classNameとstyleが同じobjectに格納されつつmemo化されていますが、それぞれ依存する変数が違うため、memo化の効率が悪くなっています。
コードの距離的にも近いため、ここでまとめるメリットが少ないのでそれぞれ別でmemo化するように修正しています
| const onClickTrigger = useCallback((rect: Rect) => { | ||
| const newActive = !active | ||
|
|
||
| setActive(newActive) | ||
|
|
||
| if (newActive) { | ||
| setTriggerRect(rect) | ||
| } | ||
| }, []) | ||
|
|
||
| const onClickCloser = useCallback(() => { | ||
| setActive(false) | ||
|
|
||
| // return focus to the Trigger | ||
| getFirstTabbable(triggerElementRef)?.focus() | ||
| }, []) |
There was a problem hiding this comment.
完全にこのコンポーネント内のhookにしか依存していないため、関数の再作成は不要でした。
useCallbackでmemo化するよう修正しています
yagimushi
force-pushed
the
chore-refactoring-Dropdown
branch
from
59c7e36 to
ec47900
Compare
commit: |
yagimushi
force-pushed
the
chore-refactoring-Dropdown
branch
from
ec47900 to
94ebff1
Compare
AtsushiM
requested review from
oti and
yuzuru-akiyama
and removed request for
a team
関連URL
概要
変更内容
確認方法