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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
81ee30b
chore: useThemeのformatを整理
AtsushiM Feb 3, 2025
a538169
chore: _isChildPortalの内部の文字列化処理ををtoStringに変更
AtsushiM Feb 3, 2025
8b74670
chore: _isChildPortalの文字列比較処理を適切にフィルタリング
AtsushiM Feb 4, 2025
a21e23c
chore: _isChildPortalの比較処理を正規表現にすることで高速化
AtsushiM Feb 4, 2025
dd08b36
chore: dataset.portalChildOfの設定を調整
AtsushiM Feb 4, 2025
e4be2b9
chore: isEventIncludedParentのロジックを最適化
AtsushiM Feb 4, 2025
5aa3fe3
chore: useOuterClickのロジックを整理
AtsushiM Feb 4, 2025
6d187cf
chore: useOuterClickの早期終了を辞める
AtsushiM Feb 4, 2025
a9d74b7
chore: useHandleEscapeのロジックを整理
AtsushiM Feb 4, 2025
df24151
chore: useHandleEscapeのkey判定を正規表現二変更
AtsushiM Feb 4, 2025
b7c1e8d
chore: useClickのisEventIncludedParentを最適化
AtsushiM Feb 4, 2025
d21c0a2
chore: useClickのmemo化を調整
AtsushiM Feb 4, 2025
60684a4
chore: useDeviceのformatを調整
AtsushiM Feb 4, 2025
5a177c1
Merge branch 'master' of https://github.com/kufu/smarthr-ui into chor…
AtsushiM Feb 4, 2025
1ac66a5
chore: fix ci
AtsushiM Feb 4, 2025
757956c
Merge branch 'master' of https://github.com/kufu/smarthr-ui into chor…
AtsushiM Feb 13, 2025
aa9b87c
chore: fix ci
AtsushiM Feb 13, 2025
4a77d14
Merge branch 'master' of https://github.com/kufu/smarthr-ui into chor…
AtsushiM Mar 2, 2025
28577b1
Merge branch 'master' of https://github.com/kufu/smarthr-ui into chor…
AtsushiM Mar 2, 2025
4f36150
Merge branch 'master' of https://github.com/kufu/smarthr-ui into chor…
AtsushiM Mar 14, 2025
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 @@ -272,7 +272,8 @@ const ActualMultiComboBox = <T,>(
return textColor.grey
}, [disabled, isFocused])

useOuterClick([outerRef, listBoxRef], blur)
const outerClickRef = useMemo(() => [outerRef, listBoxRef], [outerRef, listBoxRef])
useOuterClick(outerClickRef, blur)

useEffect(() => {
if (highlighted) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ const ActualSingleComboBox = <T,>(
}, [disabled, isFocused])

useClick(
[outerRef, listBoxRef, clearButtonRef],
useMemo(() => [outerRef, listBoxRef, clearButtonRef], [outerRef, listBoxRef, clearButtonRef]),
useCallback(() => {
if (!isFocused && onSelect && !selectedItem && defaultItem) {
onSelect(defaultItem)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ export const DatePicker = forwardRef<HTMLInputElement, Props & InputAttributes>(
}, [value, isInputFocused, dateToString, dateToAlternativeFormat, stringToDate])

useOuterClick(
[inputWrapperRef, calendarPortalRef],
useMemo(() => [inputWrapperRef, calendarPortalRef], [inputWrapperRef, calendarPortalRef]),
useCallback(() => {
switchCalendarVisibility(false)
}, [switchCalendarVisibility]),
Expand Down
23 changes: 12 additions & 11 deletions packages/smarthr-ui/src/hooks/useClick.ts
Original file line number Diff line number Diff line change
@@ -1,34 +1,35 @@
import { RefObject, useCallback, useEffect } from 'react'
import { RefObject, useEffect } from 'react'

export function useClick(
innerRefs: Array<RefObject<HTMLElement>>,
innerCallback: (e: MouseEvent) => void,
outerCallback: (e: MouseEvent) => void,
) {
const handleClick = useCallback(
(e: MouseEvent) => {
useEffect(() => {
const handleClick = (e: MouseEvent) => {
if (innerRefs.some((target) => isEventIncludedParent(e, target.current))) {
innerCallback(e)

return
}

outerCallback(e)
},
// spread innerRefs to compare deps one by one
// eslint-disable-next-line react-hooks/exhaustive-deps
[...innerRefs, innerCallback, outerCallback],
)
}

useEffect(() => {
window.addEventListener('click', handleClick)

return () => {
window.removeEventListener('click', handleClick)
}
}, [handleClick])
}, [innerRefs, innerCallback, outerCallback])
Comment on lines +8 to +24
Copy link
Member Author

Choose a reason for hiding this comment

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

handleClickはuseEffectの中でしか利用されていないため、ひとまとめにしています。
依存関係の配列も展開は微妙なのでmemo化したものが渡されてくるように修正しています

}

function isEventIncludedParent(e: MouseEvent, parent: Element | null): boolean {
if (!parent) return false

const path = e.composedPath()
if (path.length === 0 || !parent) return false

if (path.length === 0) return false
Comment on lines +28 to +32
Copy link
Member Author

Choose a reason for hiding this comment

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

pathの生成が不要なパターンに対するチェックを最適化しています


return path.includes(parent)
}
5 changes: 1 addition & 4 deletions packages/smarthr-ui/src/hooks/useDevice/useDevice.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,4 @@ import { createContext, useContext } from 'react'

export const DeviceContext = createContext<boolean | null>(null)

export const useDevice = () => {
const isNarrowView = useContext(DeviceContext)
return { isNarrowView }
}
export const useDevice = () => ({ isNarrowView: useContext(DeviceContext) })
23 changes: 12 additions & 11 deletions packages/smarthr-ui/src/hooks/useHandleEscape.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
import { useCallback, useEffect } from 'react'
import { useEffect } from 'react'

// https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key
// Esc is a IE/Edge specific value
const ESCAPE_KEY_REGEX = /^Esc(ape)?$/

export const useHandleEscape = (cb: () => void) => {
const handleKeyPress = useCallback(
(e: KeyboardEvent) => {
// https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key
// Esc is a IE/Edge specific value
if (e.key === 'Escape' || e.key === 'Esc') {
useEffect(() => {
const handleKeyPress = (e: KeyboardEvent) => {
if (ESCAPE_KEY_REGEX.test(e.key)) {
cb()
}
},
[cb],
)
useEffect(() => {
}

document.addEventListener('keydown', handleKeyPress)

return () => document.removeEventListener('keydown', handleKeyPress)
}, [handleKeyPress])
}, [cb])
Copy link
Member Author

Choose a reason for hiding this comment

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

handleKeyPressはuseEffect中でしか利用されていないため、ひとまとめにしています

}
31 changes: 15 additions & 16 deletions packages/smarthr-ui/src/hooks/useOuterClick.ts
Original file line number Diff line number Diff line change
@@ -1,31 +1,30 @@
import { RefObject, useCallback, useEffect } from 'react'
import { RefObject, useEffect } from 'react'

export function useOuterClick(
targets: Array<RefObject<HTMLElement>>,
callback: (e: MouseEvent) => void,
) {
const handleOuterClick = useCallback(
(e: MouseEvent) => {
if (targets.some((target) => isEventIncludedParent(e, target.current))) {
return
useEffect(() => {
const handleOuterClick = (e: MouseEvent) => {
if (targets.every((target) => isEventExcludedParent(e, target.current))) {
callback(e)
}
callback(e)
},
// spread targets to compare deps one by one
// eslint-disable-next-line react-hooks/exhaustive-deps
[...targets, callback],
)
}
Comment on lines -7 to +12
Copy link
Member Author

Choose a reason for hiding this comment

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

早期returnが直後の処理の逆転条件でしかないため、理解がワンテンポ遅れることになり、デメリットしかない状態になっています。
条件を逆転させることでわかりやすくしています

Copy link
Collaborator

Choose a reason for hiding this comment

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

理解がワンテンポ遅れるのは個人の感覚かなと思います。
早期 return は主要な処理を後半に持ってくることでわかりやすくなると私は思います。

Copy link
Member Author

Choose a reason for hiding this comment

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

早期 return は主要な処理を後半に持ってくることでわかりやすくなると私は思います。

すべての早期returnがそう、という話ではなく、今回の場合は以下の様になっていたためです。

if (a) {
  return
}

return result

上記コードは本質的には resultは!aの場合実行する という条件の逆転を行っているだけです。
書いた人にはわかりやすいかもしれませんが、このコードを読む他の開発者は aの条件を逆転させてresultが実行される真の条件を考える 必要が発生します。
aの条件が複雑化するごとにこの反転のコストは大きくなります。

if (!a) {
 return result
}

こっちのほうが素直では?という話です。
書きかけですが https://codimd.kufu.tools/HmUCuZACTKC2Kao-QoCrPw#%E6%97%A9%E6%9C%9Freturn%E3%81%AF%E5%BF%85%E8%A6%81%E3%81%AA%E7%AE%87%E6%89%80%E3%81%AE%E3%81%BF%E3%81%A7%E4%BD%BF%E3%81%86%E3%81%B9%E3%81%8D%E3%81%A7%E3%81%82%E3%82%8B とかで詳しく書いてます。


useEffect(() => {
window.addEventListener('click', handleOuterClick)

return () => {
window.removeEventListener('click', handleOuterClick)
}
}, [handleOuterClick])
}, [callback, targets])
}

function isEventIncludedParent(e: MouseEvent, parent: Element | null): boolean {
function isEventExcludedParent(e: MouseEvent, parent: Element | null): boolean {
if (!parent) return false

const path = e.composedPath()
if (path.length === 0 || !parent) return false
return path.includes(parent)

if (path.length === 0) return false

return !path.includes(parent)
}
44 changes: 28 additions & 16 deletions packages/smarthr-ui/src/hooks/usePortal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,15 @@ export function usePortal() {
const [portalRoot, setPortalRoot] = useState<HTMLDivElement | null>(null)
const currentSeq = useMemo(() => ++portalSeq, [])
const parent = useContext(ParentContext)
const parentSeqs = parent.seqs.concat(currentSeq)

const calculatedSeqs = useMemo(() => {
const parentSeqs = parent.seqs.concat(currentSeq)

return {
parentSeqs,
portalChildOf: parentSeqs.join(','),
}
}, [currentSeq, parent.seqs])
Comment on lines +29 to +36
Copy link
Member Author

Choose a reason for hiding this comment

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

このファイルがやっていることはややこしいのですが、ざっくりいうと以下のとおりです

  • アプリ全体でのPortalにシーケンスnoを振ります
  • Portalを開く毎に紐づくnoをdata属性に追加し、現在開かれているportalが分かる状態にします
  • 上記を利用して、特定のPortalがなにかのPortalの子になっているか?をチェックできるようにします

このチェックのため、data属性を利用していますが、data属性は基本stringしか突っ込めないので、joinでつなげるようにしています


useEnhancedEffect(() => {
// Next.jsのhydration error回避のため、初回レンダリング時にdivを作成する
Expand All @@ -36,36 +44,37 @@ export function usePortal() {
if (!portalRoot) {
return
}
portalRoot.dataset.portalChildOf = parentSeqs.join(',')

portalRoot.dataset.portalChildOf = calculatedSeqs.portalChildOf
document.body.appendChild(portalRoot)

return () => {
document.body.removeChild(portalRoot)
}
// spread parentSeqs array for deps
}, [portalRoot, ...parentSeqs])
}, [portalRoot, calculatedSeqs.portalChildOf])

const isChildPortal = useCallback(
(element: HTMLElement | null) => _isChildPortal(element, currentSeq),
(element: HTMLElement | null) => _isChildPortal(element, new RegExp(`(^|,)${currentSeq}(,|$)`)),
[currentSeq],
)

const PortalParentProvider: FC<{ children: ReactNode }> = useCallback(
({ children }) => {
const value: ParentContextValue = {
seqs: parentSeqs,
seqs: calculatedSeqs.parentSeqs,
}

return <ParentContext.Provider value={value}>{children}</ParentContext.Provider>
},
// spread parentSeqs array for deps
// eslint-disable-next-line react-hooks/exhaustive-deps
[...parentSeqs],
[calculatedSeqs.parentSeqs],
)

const wrappedCreatePortal = useCallback(
(children: ReactNode) => {
if (portalRoot === null) {
return null
}

return createPortal(children, portalRoot)
},
[portalRoot],
Expand All @@ -82,12 +91,15 @@ export function usePortal() {
}
}

function _isChildPortal(
element: HTMLElement | SVGElement | null,
parentPortalSeq: number,
): boolean {
function _isChildPortal(element: HTMLElement | SVGElement | null, seqRegex: RegExp): boolean {
if (!element) return false
const childOf = element.dataset?.portalChildOf || ''
const includesSeq = childOf.split(',').includes(String(parentPortalSeq))
return includesSeq || _isChildPortal(element.parentElement, parentPortalSeq)

let includesSeq = false
const childOf = element.dataset?.portalChildOf

if (childOf) {
includesSeq = seqRegex.test(childOf)
}
Comment on lines -90 to +102
Copy link
Member Author

Choose a reason for hiding this comment

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

これまでのロジックでは ,つなぎの数値をsplitして比較していましたが、シーケンス番号は数値を,でつなげたものであるため、正規表現でsplitしなくてもチェック可能にしました。

https://github.com/kufu/smarthr-ui/pull/5361/files#diff-606e53788a9b6453997da495ed36be27a5678880d9c1151e4cc056400907a121R57


return includesSeq || _isChildPortal(element.parentElement, seqRegex)
}
5 changes: 1 addition & 4 deletions packages/smarthr-ui/src/hooks/useTheme.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,4 @@ import { CreatedTheme } from '../themes/createTheme'

export type Theme = CreatedTheme

export const useTheme = () => {
const theme = useContext(ThemeContext)
return theme
}
export const useTheme = () => useContext(ThemeContext)