chore: DropdownMenuButton の内部ロジックを整理する #5314
Open
+142
−109
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yagimushi
force-pushed
the
chore-refactoring-DropdownMenuButton
branch
3 times, most recently
from
af4bfbe to
c240c5e
Compare
commit: |
AtsushiM
commented
Jan 22, 2025
Comment on lines
-87
to
-92
| if (['Up', 'ArrowUp', 'Left', 'ArrowLeft'].includes(e.key)) { | ||
| moveFocus(-1, enabledItems, focusedIndex, hoveredItem) | ||
| } | ||
|
|
||
| if (['Down', 'ArrowDown', 'Right', 'ArrowRight'].includes(e.key)) { | ||
| moveFocus(1, enabledItems, focusedIndex, hoveredItem) |
There was a problem hiding this comment.
直前で計算している allItems ですが、押したキーが合致しない場合、完全に無駄処理になります。
チェックする順序を調整して無駄処理が発生しないようにしています
AtsushiM
commented
Jan 22, 2025
| }, | ||
| ) | ||
|
|
||
| if (['Up', 'ArrowUp', 'Left', 'ArrowLeft'].includes(e.key)) { |
There was a problem hiding this comment.
配列に対してincludesを実行するより、正規表現一発でのチェックのほうが高速のため調整しています
AtsushiM
commented
Jan 22, 2025
Comment on lines
+50
to
+60
| 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 |
There was a problem hiding this comment.
元々は関数内にロジックが隠蔽されていましたが、変数の汚染などもないし、nextIndexに値を入れるだけだったのでただのif-elseにしました
AtsushiM
commented
Jan 22, 2025
| @@ -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)) | |||
There was a problem hiding this comment.
matchesDisabledState を呼び出すための無名関数を生成していましたが、引数が完全一致のため、無意味でした。
matchesDisabledStateを直接参照渡しするようにしました
AtsushiM
commented
Jan 22, 2025
Comment on lines
+56
to
+60
| 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} |
There was a problem hiding this comment.
childrenに対してReactNodeが渡されますが、たいていはstringになるはずなのでmemo化しました
AtsushiM
commented
Jan 22, 2025
Comment on lines
128
to
142
| const TriggerLabel = 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="候補を開く" />, | ||
| ) |
There was a problem hiding this comment.
useMemoされていましたが、内容がReactNodeなので、より適切なReact.memoを使うようにしています
AtsushiM
commented
Jan 22, 2025
| @@ -70,59 +70,88 @@ export const DropdownMenuButton: FC<Props & ElementProps> = ({ | |||
| label, | |||
| children, | |||
| triggerSize, | |||
| onlyIconTrigger = false, | |||
| triggerIcon: TriggerIcon, | |||
| onlyIconTrigger, | |||
There was a problem hiding this comment.
比較しかしていないため、初期値代入は不要でした
yagimushi
force-pushed
the
chore-refactoring-DropdownMenuButton
branch
from
c240c5e to
0590fe2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
関連URL
概要
変更内容
確認方法