-
Notifications
You must be signed in to change notification settings - Fork 141
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: Dialog周辺コンポーネントのロジックを整理する #5318
base: master
Are you sure you want to change the base?
Changes from all commits
cf3f179
0cf902b
56a67a9
6d82769
d397f31
e7d51c8
d992533
2e54a1d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,6 @@ | ||
'use client' | ||
|
||
import React, { | ||
ComponentProps, | ||
FC, | ||
PropsWithChildren, | ||
RefObject, | ||
useCallback, | ||
useMemo, | ||
useRef, | ||
} from 'react' | ||
import React, { ComponentProps, FC, PropsWithChildren, RefObject, useMemo, useRef } from 'react' | ||
import { tv } from 'tailwind-variants' | ||
|
||
import { useHandleEscape } from '../../hooks/useHandleEscape' | ||
|
@@ -68,9 +60,7 @@ const dialogContentInner = tv({ | |
|
||
export const DialogContentInner: FC<DialogContentInnerProps & ElementProps> = ({ | ||
onClickOverlay, | ||
onPressEscape = () => { | ||
/* noop */ | ||
}, | ||
onPressEscape, | ||
isOpen, | ||
id, | ||
width, | ||
|
@@ -81,44 +71,39 @@ export const DialogContentInner: FC<DialogContentInnerProps & ElementProps> = ({ | |
className, | ||
...rest | ||
}) => { | ||
const { layoutStyleProps, innerStyle, backgroundStyle } = useMemo(() => { | ||
const { layoutStyle, innerStyle, backgroundStyle } = useMemo(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style属性とclass属性を同時にmemo化するメリットがない箇所だったので分割しました |
||
const { layout, inner, background } = dialogContentInner() | ||
const actualWidth = typeof width === 'number' ? `${width}px` : width | ||
|
||
return { | ||
layoutStyleProps: { | ||
className: layout(), | ||
style: { | ||
width: actualWidth ?? undefined, | ||
}, | ||
}, | ||
layoutStyle: layout(), | ||
innerStyle: inner({ className }), | ||
backgroundStyle: background(), | ||
} | ||
}, [className, width]) | ||
}, [className]) | ||
const styleAttr = useMemo(() => { | ||
const actualWidth = typeof width === 'number' ? `${width}px` : width | ||
|
||
if (!actualWidth) { | ||
return undefined | ||
} | ||
|
||
return { | ||
width: actualWidth, | ||
} | ||
}, [width]) | ||
|
||
const innerRef = useRef<HTMLDivElement>(null) | ||
|
||
useHandleEscape( | ||
useCallback(() => { | ||
if (!isOpen) { | ||
return | ||
} | ||
onPressEscape() | ||
}, [isOpen, onPressEscape]), | ||
useMemo(() => (onPressEscape && isOpen ? onPressEscape : undefined), [isOpen, onPressEscape]), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. なるべくキャッシュが有効になるよう、onPressEscapeが存在し、かつ実行するひつようがあるときだけ参照渡しするようにしました |
||
) | ||
|
||
const handleClickOverlay = useCallback(() => { | ||
if (isOpen && onClickOverlay) { | ||
onClickOverlay() | ||
} | ||
}, [isOpen, onClickOverlay]) | ||
|
||
useBodyScrollLock(isOpen) | ||
|
||
return ( | ||
<DialogOverlap isOpen={isOpen}> | ||
<div {...layoutStyleProps} id={id}> | ||
{/* eslint-disable-next-line smarthr/a11y-delegate-element-has-role-presentation */} | ||
<div onClick={handleClickOverlay} className={backgroundStyle} role="presentation" /> | ||
<div id={id} className={layoutStyle} style={styleAttr}> | ||
<Overlay isOpen={isOpen} onClickOverlay={onClickOverlay} className={backgroundStyle} /> | ||
<div | ||
{...rest} | ||
ref={innerRef} | ||
|
@@ -134,3 +119,15 @@ export const DialogContentInner: FC<DialogContentInnerProps & ElementProps> = ({ | |
</DialogOverlap> | ||
) | ||
} | ||
|
||
const Overlay = React.memo< | ||
Pick<DialogContentInnerProps, 'onClickOverlay' | 'isOpen'> & { className: string } | ||
>(({ onClickOverlay, isOpen, className }) => { | ||
const handleClickOverlay = useMemo( | ||
() => (onClickOverlay && isOpen ? onClickOverlay : undefined), | ||
[isOpen, onClickOverlay], | ||
) | ||
|
||
// eslint-disable-next-line smarthr/a11y-delegate-element-has-role-presentation | ||
return <div onClick={handleClickOverlay} className={className} role="presentation" /> | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import React from 'react' | ||
import React, { useMemo } from 'react' | ||
import { tv } from 'tailwind-variants' | ||
|
||
import { Heading, HeadingTagTypes } from '../Heading' | ||
|
@@ -25,8 +25,9 @@ const dialogHeader = tv({ | |
], | ||
}) | ||
|
||
export const DialogHeader: React.FC<Props> = ({ title, subtitle, titleTag, titleId }) => { | ||
const style = dialogHeader() | ||
export const DialogHeader = React.memo<Props>(({ title, subtitle, titleTag, titleId }) => { | ||
const style = useMemo(() => dialogHeader(), []) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DialogのHeaderはほぼ再レンダリングが不要な場合が多いと予想されるため、丸ごとmemo化しました |
||
|
||
return ( | ||
// eslint-disable-next-line smarthr/a11y-heading-in-sectioning-content | ||
<Heading tag={titleTag} className={style}> | ||
|
@@ -42,4 +43,4 @@ export const DialogHeader: React.FC<Props> = ({ title, subtitle, titleTag, title | |
</Stack> | ||
</Heading> | ||
) | ||
} | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,32 +1,32 @@ | ||
'use client' | ||
|
||
import React, { PropsWithChildren, createContext, useState } from 'react' | ||
import React, { PropsWithChildren, createContext, useCallback, useState } from 'react' | ||
|
||
type DialogContextType = { | ||
onClickTrigger: () => void | ||
onClickClose: () => void | ||
active: boolean | ||
} | ||
|
||
const noop = () => undefined | ||
export const DialogContext = createContext<DialogContextType>({ | ||
onClickTrigger: () => { | ||
/* noop */ | ||
}, | ||
onClickClose: () => { | ||
/* noop */ | ||
}, | ||
onClickTrigger: noop, | ||
onClickClose: noop, | ||
active: false, | ||
}) | ||
|
||
export const DialogWrapper: React.FC<PropsWithChildren> = (props) => { | ||
const [active, setActive] = useState(false) | ||
|
||
const onClickTrigger = useCallback(() => setActive(true), []) | ||
const onClickClose = useCallback(() => setActive(false), []) | ||
Comment on lines
+21
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 再生成が完全に不要な箇所だったため、memo化しています |
||
|
||
return ( | ||
<DialogContext.Provider | ||
{...props} | ||
value={{ | ||
onClickTrigger: () => setActive(true), | ||
onClickClose: () => setActive(false), | ||
onClickTrigger, | ||
onClickClose, | ||
active, | ||
}} | ||
/> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,29 @@ | ||
import { useCallback, useEffect } from 'react' | ||
import { useEffect, useMemo } from 'react' | ||
|
||
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') { | ||
cb() | ||
} | ||
}, | ||
const ESCAPE_KEY_REGEX = /^Esc(ape)?$/ | ||
|
||
export const useHandleEscape = (cb?: () => void) => { | ||
const handleKeyPress = useMemo( | ||
() => | ||
cb | ||
? (e: KeyboardEvent) => { | ||
// https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key | ||
// Esc is a IE/Edge specific value | ||
if (ESCAPE_KEY_REGEX.test(e.key)) { | ||
cb() | ||
} | ||
} | ||
: null, | ||
[cb], | ||
) | ||
|
||
useEffect(() => { | ||
if (!handleKeyPress) { | ||
return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 実行する必要が一切ない場合でもevent listnerが実行されるパターンが存在したため、早期終了を追加しました |
||
} | ||
|
||
document.addEventListener('keydown', handleKeyPress) | ||
|
||
return () => document.removeEventListener('keydown', handleKeyPress) | ||
}, [handleKeyPress]) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onPressEscapeが存在しない場合、そもそも実行する必要すらない箇所だったので、初期化を削除しました