[Feat] #70 - 공통 모달(Modal/Dialog) 컴포넌트 시스템 구현 및 예제 추가#83
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdded a new Modal component system (Modal, ModalHeader, ModalContent, ModalFooter) with portal, Escape/overlay close, and scroll locking; exported modal and inputBox components from the UI barrel; added modal Storybook examples; simplified an isAllChecked boolean in an existing lists story. Changes
Sequence Diagram(s)sequenceDiagram
participant Story as Story / Caller
participant Modal as Modal Component
participant Body as document.body (Portal)
participant User as User
rect rgba(200,200,255,0.5)
Story->>Modal: render with isOpen=true
Modal->>Body: create portal, insert overlay + dialog
end
rect rgba(200,255,200,0.5)
User->>Modal: click overlay (backdrop)
Modal->>Modal: if target === backdrop -> call onClose()
Modal->>Body: remove portal, restore body overflow
Story->>Modal: receive onClose, update state
end
rect rgba(255,200,200,0.5)
User->>Modal: press Escape
Modal->>Modal: keydown handler detects Escape -> call onClose()
Modal->>Body: remove portal, restore body overflow
Story->>Modal: receive onClose, update state
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/shared/ui/modal/modal.stories.tsx (2)
139-146: Same issue with unused args.Same as
FilterModal— the args are not wired toProfileEditModalExample.Remove unused args
export const ProfileEditModal: Story = { render: () => <ProfileEditModalExample />, - args: { - isOpen: true, - onClose: () => {}, - children: null, - }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/modal/modal.stories.tsx` around lines 139 - 146, The story ProfileEditModal defines args but does not pass them into ProfileEditModalExample, so the props (isOpen, onClose, children) are unused; update the story's render to accept args and forward them to ProfileEditModalExample (e.g., change render to (args) => <ProfileEditModalExample {...args} />) or otherwise wire the args into the component, mirroring the fix used for FilterModal so the story actually uses the provided props.
73-80: Story args are not used and may confuse Storybook controls.The
argsobject specifiesisOpen: true, butFilterModalExampleinitializes withuseState(false). Since the render function ignores these args entirely, they serve no purpose and may mislead users expecting Storybook controls to work.Consider either removing the unused args or passing them to the example component.
Option 1: Remove unused args
export const FilterModal: Story = { render: () => <FilterModalExample />, - args: { - isOpen: true, - onClose: () => {}, - children: null, - }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/modal/modal.stories.tsx` around lines 73 - 80, The Storybook story FilterModal declares args (isOpen, onClose, children) but the render uses FilterModalExample which ignores them (its internal useState(false)); either remove the unused args from the FilterModal story OR wire the args through: change the render to accept args and pass them into FilterModalExample (and update FilterModalExample to accept props like isOpen and onClose instead of hardcoded useState), ensuring the story's args control the modal state and Storybook controls behave as expected.src/shared/ui/modal/modal.tsx (2)
72-79: Consider addingaria-labelledbyfor better accessibility.The dialog has
role="dialog"andaria-modal="true", but lacksaria-labelledbyto associate it with the title. This helps screen readers announce the dialog purpose.♿ Proposed enhancement
Generate a stable ID (e.g., via
useId()) and wire it betweenModalandModalHeader:+import React, { useEffect, useId } from "react"; ... export const Modal = ({ isOpen, onClose, children, className }: ModalProps) => { + const titleId = useId(); ... return createPortal( <div className={overlayBaseClasses} onClick={(e) => e.target === e.currentTarget && onClose()}> - <div role="dialog" aria-modal="true" className={getModalClasses(className)}> - {children} + <div role="dialog" aria-modal="true" aria-labelledby={titleId} className={getModalClasses(className)}> + {React.Children.map(children, child => + React.isValidElement(child) && child.type === ModalHeader + ? React.cloneElement(child, { titleId }) + : child + )} </div> </div>, document.body, ); };Then in
ModalHeader, applyid={titleId}to the<h2>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/modal/modal.tsx` around lines 72 - 79, The dialog lacks an aria-labelledby so screen readers can't associate the title; generate a stable id in the Modal component (useId or a stable id provider) and pass it down as a prop (e.g., titleId) to the ModalHeader, then add aria-labelledby={titleId} to the div with role="dialog" in Modal and ensure ModalHeader sets id={titleId} on its <h2>; update the createPortal return (the div with role="dialog" using getModalClasses) to include aria-labelledby and thread the titleId prop through any wrapper components that render ModalHeader.
82-96: Class joining can produce extra spaces whenclassNameis undefined.Using
[headerBaseClasses, className].join(" ")produces a trailing space whenclassNameisundefined(e.g.,"flex items-center ... "). This is cosmetic but inconsistent withgetModalClasseswhich filters properly.♻️ Consistent class joining
export const ModalHeader = ({ title, onClose, children, className }: ModalHeaderProps) => ( - <div className={[headerBaseClasses, className].join(" ")}> + <div className={[headerBaseClasses, className].filter(Boolean).join(" ")}> ... </div> ); export const ModalContent = ({ children, className }: ModalContentProps) => ( - <div className={[contentBaseClasses, className].join(" ")}>{children}</div> + <div className={[contentBaseClasses, className].filter(Boolean).join(" ")}>{children}</div> ); export const ModalFooter = ({ children, className }: ModalFooterProps) => ( - <div className={[footerBaseClasses, className].join(" ")}>{children}</div> + <div className={[footerBaseClasses, className].filter(Boolean).join(" ")}>{children}</div> );Alternatively, use the existing
cnutility from@/shared/lib/utilsfor consistency with other components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/modal/modal.tsx` around lines 82 - 96, The ModalHeader component builds its class string using [headerBaseClasses, className].join(" "), which leaves an extra space when className is undefined; update ModalHeader to build classes consistently by using the existing cn utility (or by filtering falsy values) so that headerBaseClasses and the optional className are combined without producing trailing spaces — locate ModalHeader and replace the join logic that references headerBaseClasses and className with a call to cn(headerBaseClasses, className) (or equivalent filtering).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/shared/ui/modal/modal.tsx`:
- Around line 54-68: The effect in modal.tsx (the useEffect that watches
isOpen/onClose and uses handleEsc) clobbers any pre-existing
document.body.style.overflow and breaks nested modals; change it to preserve and
restore the original overflow and be stack-safe by: capture and store the
original overflow value when the first modal opens, use a module-level counter
(e.g., modalOpenCount) that increments when a modal opens and decrements on
close, only set document.body.style.overflow = "hidden" when the counter
transitions 0→1, and only restore the saved original overflow when the counter
transitions 1→0; keep the existing keydown handling (handleEsc) but ensure
addEventListener/removeEventListener remain tied to the individual modal's
lifecycle.
---
Nitpick comments:
In `@src/shared/ui/modal/modal.stories.tsx`:
- Around line 139-146: The story ProfileEditModal defines args but does not pass
them into ProfileEditModalExample, so the props (isOpen, onClose, children) are
unused; update the story's render to accept args and forward them to
ProfileEditModalExample (e.g., change render to (args) =>
<ProfileEditModalExample {...args} />) or otherwise wire the args into the
component, mirroring the fix used for FilterModal so the story actually uses the
provided props.
- Around line 73-80: The Storybook story FilterModal declares args (isOpen,
onClose, children) but the render uses FilterModalExample which ignores them
(its internal useState(false)); either remove the unused args from the
FilterModal story OR wire the args through: change the render to accept args and
pass them into FilterModalExample (and update FilterModalExample to accept props
like isOpen and onClose instead of hardcoded useState), ensuring the story's
args control the modal state and Storybook controls behave as expected.
In `@src/shared/ui/modal/modal.tsx`:
- Around line 72-79: The dialog lacks an aria-labelledby so screen readers can't
associate the title; generate a stable id in the Modal component (useId or a
stable id provider) and pass it down as a prop (e.g., titleId) to the
ModalHeader, then add aria-labelledby={titleId} to the div with role="dialog" in
Modal and ensure ModalHeader sets id={titleId} on its <h2>; update the
createPortal return (the div with role="dialog" using getModalClasses) to
include aria-labelledby and thread the titleId prop through any wrapper
components that render ModalHeader.
- Around line 82-96: The ModalHeader component builds its class string using
[headerBaseClasses, className].join(" "), which leaves an extra space when
className is undefined; update ModalHeader to build classes consistently by
using the existing cn utility (or by filtering falsy values) so that
headerBaseClasses and the optional className are combined without producing
trailing spaces — locate ModalHeader and replace the join logic that references
headerBaseClasses and className with a call to cn(headerBaseClasses, className)
(or equivalent filtering).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55e41a96-5f9e-4690-b04a-526e97ae2913
📒 Files selected for processing (5)
src/shared/ui/index.tssrc/shared/ui/lists/lists.stories.tsxsrc/shared/ui/modal/index.tssrc/shared/ui/modal/modal.stories.tsxsrc/shared/ui/modal/modal.tsx
kimsman06
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/shared/ui/modal/modal.tsx (3)
10-16: UnusedtitleIdprop in ModalProps.The
titleIdprop is defined but never used in theModalcomponent—it always generates its own ID viauseId()on line 56. Either remove the prop or use it to allow consumers to override the generated ID.♻️ Option 1: Remove unused prop
export type ModalProps = { isOpen: boolean; onClose: () => void; children: React.ReactNode; className?: string; - titleId?: string; // 접근성을 위한 ID };♻️ Option 2: Use the prop as an override
-export const Modal = ({ isOpen, onClose, children, className }: ModalProps) => { +export const Modal = ({ isOpen, onClose, children, className, titleId: titleIdProp }: ModalProps) => { const generatedId = useId(); - const titleId = `modal-title-${generatedId}`; + const titleId = titleIdProp ?? `modal-title-${generatedId}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/modal/modal.tsx` around lines 10 - 16, The ModalProps includes a titleId that's never used; update the Modal component to accept and use the consumer-provided titleId as an override of the internally generated id from useId() (i.e., compute const titleId = props.titleId || generatedId) and apply that titleId to the modal title element and to aria-labelledby on the dialog, or alternatively remove titleId from ModalProps and all references if you prefer to disallow overrides; adjust the ModalProps type, the Modal component (where useId() is called) and the title/aria attributes accordingly to ensure consistency.
58-82: Consider using a ref foronCloseto avoid effect churn.The
onClosedependency causes the effect to re-run whenever the callback reference changes. If the parent doesn't memoizeonClosewithuseCallback, this will repeatedly remove/add the event listener and briefly toggle scroll lock on every render while the modal is open.A common pattern is to store
onClosein a ref that the effect reads without depending on it.♻️ Proposed fix using ref
+import React, { useEffect, useId, useRef } from "react"; ... export const Modal = ({ isOpen, onClose, children, className }: ModalProps) => { const generatedId = useId(); const titleId = `modal-title-${generatedId}`; + const onCloseRef = useRef(onClose); + onCloseRef.current = onClose; useEffect(() => { if (!isOpen) return; // ESC 키 대응 const handleEsc = (e: KeyboardEvent) => { - if (e.key === "Escape") onClose(); + if (e.key === "Escape") onCloseRef.current(); }; // 스크롤 잠금 (중첩 모달 대응) if (modalOpenCount === 0) { originalOverflow = document.body.style.overflow; document.body.style.overflow = "hidden"; } modalOpenCount++; window.addEventListener("keydown", handleEsc); return () => { modalOpenCount--; if (modalOpenCount === 0) { document.body.style.overflow = originalOverflow; } window.removeEventListener("keydown", handleEsc); }; - }, [isOpen, onClose]); + }, [isOpen]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/modal/modal.tsx` around lines 58 - 82, The effect in the Modal component re-runs whenever the onClose callback identity changes; change this by storing onClose in a ref (e.g., onCloseRef) and always calling onCloseRef.current() from the handleEsc handler so the effect no longer depends on onClose; update onCloseRef.current whenever props change (use useEffect or useLayoutEffect) and keep the main modal opening/closing effect for ESC handling, scroll lock, modalOpenCount, originalOverflow and window.addEventListener only dependent on isOpen (remove onClose from that dependency array) so event listener and scroll-lock behavior stop churning when parent re-renders.
86-92: Type checkchild.type === ModalHeadermay be fragile.Direct component reference comparisons can fail when components are wrapped (HOCs,
forwardRef,memo) or in certain bundler/minification scenarios. This is a common React pattern but worth being aware of for future maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/modal/modal.tsx` around lines 86 - 92, The equality check child.type === ModalHeader is fragile for wrapped components; update the childrenWithA11y mapping to detect ModalHeader more robustly by checking multiple identifiers: keep React.isValidElement(child) and React.cloneElement as-is, but replace the strict equality with a helper condition that compares child.type === ModalHeader || (child.type as any).displayName === (ModalHeader as any).displayName || (child.type as any).displayName === 'ModalHeader' || (child.type as any).name === 'ModalHeader'; apply this check where ModalHeaderProps and titleId are injected so wrapped/forwardRef/memo variants are handled too.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/shared/ui/modal/modal.tsx`:
- Around line 109-127: The ModalHeader currently only applies the id prop to the
conditional <h2>, causing aria-labelledby on the dialog to point to a missing
element when title is omitted; update ModalHeader so the id is applied to the
wrapper <div> (the element using getClasses(headerBaseClasses, className)) so an
element with that id always exists, and remove or avoid relying on the
conditional h2 for the id (keep the h2 as-is for semantics). This ensures
aria-labelledby targets a consistently rendered element while preserving title
rendering and styling.
---
Nitpick comments:
In `@src/shared/ui/modal/modal.tsx`:
- Around line 10-16: The ModalProps includes a titleId that's never used; update
the Modal component to accept and use the consumer-provided titleId as an
override of the internally generated id from useId() (i.e., compute const
titleId = props.titleId || generatedId) and apply that titleId to the modal
title element and to aria-labelledby on the dialog, or alternatively remove
titleId from ModalProps and all references if you prefer to disallow overrides;
adjust the ModalProps type, the Modal component (where useId() is called) and
the title/aria attributes accordingly to ensure consistency.
- Around line 58-82: The effect in the Modal component re-runs whenever the
onClose callback identity changes; change this by storing onClose in a ref
(e.g., onCloseRef) and always calling onCloseRef.current() from the handleEsc
handler so the effect no longer depends on onClose; update onCloseRef.current
whenever props change (use useEffect or useLayoutEffect) and keep the main modal
opening/closing effect for ESC handling, scroll lock, modalOpenCount,
originalOverflow and window.addEventListener only dependent on isOpen (remove
onClose from that dependency array) so event listener and scroll-lock behavior
stop churning when parent re-renders.
- Around line 86-92: The equality check child.type === ModalHeader is fragile
for wrapped components; update the childrenWithA11y mapping to detect
ModalHeader more robustly by checking multiple identifiers: keep
React.isValidElement(child) and React.cloneElement as-is, but replace the strict
equality with a helper condition that compares child.type === ModalHeader ||
(child.type as any).displayName === (ModalHeader as any).displayName ||
(child.type as any).displayName === 'ModalHeader' || (child.type as any).name
=== 'ModalHeader'; apply this check where ModalHeaderProps and titleId are
injected so wrapped/forwardRef/memo variants are handled too.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61585652-281e-4d5b-8809-38e79975cb30
📒 Files selected for processing (2)
src/shared/ui/modal/modal.stories.tsxsrc/shared/ui/modal/modal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/shared/ui/modal/modal.stories.tsx
Comment on lines
+109
to
+127
| export const ModalHeader = ({ title, onClose, children, className, id }: ModalHeaderProps) => ( | ||
| <div className={getClasses(headerBaseClasses, className)}> | ||
| {title && ( | ||
| <h2 id={id} className="text-[20px] font-bold text-[var(--color-gray-900,#111)]"> | ||
| {title} | ||
| </h2> | ||
| )} | ||
| {children} | ||
| {onClose && ( | ||
| <button | ||
| onClick={onClose} | ||
| className="p-1 rounded-md text-[var(--color-gray-400,#999)] hover:text-[var(--color-gray-600,#666)] hover:bg-[var(--color-gray-100,#f2f2f2)] transition-colors" | ||
| aria-label="Close modal" | ||
| > | ||
| <XIcon size={24} /> | ||
| </button> | ||
| )} | ||
| </div> | ||
| ); |
Contributor
There was a problem hiding this comment.
aria-labelledby may reference non-existent element when title is omitted.
The id prop is only applied to the <h2> which renders conditionally when title is provided. If ModalHeader is used with only children (no title), the aria-labelledby on the modal dialog will point to a non-existent element.
Consider applying the id to the wrapper <div> or to an element that always renders.
🛡️ Proposed fix: apply id to wrapper
export const ModalHeader = ({ title, onClose, children, className, id }: ModalHeaderProps) => (
- <div className={getClasses(headerBaseClasses, className)}>
+ <div id={id} className={getClasses(headerBaseClasses, className)}>
{title && (
- <h2 id={id} className="text-[20px] font-bold text-[var(--color-gray-900,`#111`)]">
+ <h2 className="text-[20px] font-bold text-[var(--color-gray-900,`#111`)]">
{title}
</h2>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const ModalHeader = ({ title, onClose, children, className, id }: ModalHeaderProps) => ( | |
| <div className={getClasses(headerBaseClasses, className)}> | |
| {title && ( | |
| <h2 id={id} className="text-[20px] font-bold text-[var(--color-gray-900,#111)]"> | |
| {title} | |
| </h2> | |
| )} | |
| {children} | |
| {onClose && ( | |
| <button | |
| onClick={onClose} | |
| className="p-1 rounded-md text-[var(--color-gray-400,#999)] hover:text-[var(--color-gray-600,#666)] hover:bg-[var(--color-gray-100,#f2f2f2)] transition-colors" | |
| aria-label="Close modal" | |
| > | |
| <XIcon size={24} /> | |
| </button> | |
| )} | |
| </div> | |
| ); | |
| export const ModalHeader = ({ title, onClose, children, className, id }: ModalHeaderProps) => ( | |
| <div id={id} className={getClasses(headerBaseClasses, className)}> | |
| {title && ( | |
| <h2 className="text-[20px] font-bold text-[var(--color-gray-900,`#111`)]"> | |
| {title} | |
| </h2> | |
| )} | |
| {children} | |
| {onClose && ( | |
| <button | |
| onClick={onClose} | |
| className="p-1 rounded-md text-[var(--color-gray-400,`#999`)] hover:text-[var(--color-gray-600,`#666`)] hover:bg-[var(--color-gray-100,`#f2f2f2`)] transition-colors" | |
| aria-label="Close modal" | |
| > | |
| <XIcon size={24} /> | |
| </button> | |
| )} | |
| </div> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/ui/modal/modal.tsx` around lines 109 - 127, The ModalHeader
currently only applies the id prop to the conditional <h2>, causing
aria-labelledby on the dialog to point to a missing element when title is
omitted; update ModalHeader so the id is applied to the wrapper <div> (the
element using getClasses(headerBaseClasses, className)) so an element with that
id always exists, and remove or avoid relying on the conditional h2 for the id
(keep the h2 as-is for semantics). This ensures aria-labelledby targets a
consistently rendered element while preserving title rendering and styling.
6 tasks
This was referenced Apr 2, 2026
5 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
[Feat] #70 - 공통 모달(Modal/Dialog) 컴포넌트 시스템 구현 및 예제 추가
🔎 What is this PR?
서비스 전반에서 사용될 레이어 팝업 시스템인 공통 모달 컴포넌트를 shared/ui 레이어에 구현했습니다. 복합 컴포넌트 패턴을
적용하여 다양한 콘텐츠 구성을 유연하게 지원하며, 웹 접근성 및 사용자 경험을 고려한 필수 인터랙션(ESC 닫기, 외부 클릭 닫기
등)을 포함합니다.
📝 Changes
📸 Screenshots
📚 Background / Context
공통화하여 개발 생산성을 높이기 위함.
✔ Checklist
Summary by CodeRabbit
New Features
Documentation