모달 컴포넌트#12
Conversation
- 커스텀훅 분리 - 유틸함수 추가
Summary of ChangesHello @Jieunsse, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 재사용 가능하고 접근성이 뛰어난 모달 컴포넌트를 시스템에 도입합니다. 이 컴포넌트는 모달의 열림/닫힘 상태를 제어하고, 외부 클릭이나 ESC 키 입력 시 자동으로 닫히며, 키보드 내비게이션 사용자를 위해 모달 내부에 포커스를 가두는 기능을 포함하여 사용자 인터페이스의 상호작용성과 접근성을 크게 향상시키는 것을 목표로 합니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| export interface ModalProps { | ||
| isOpen: boolean; | ||
| onClose: () => void; | ||
| children?: ReactNode; | ||
|
|
||
| ariaLabel: string; | ||
| ariaLabelledby?: string; | ||
| ariaDescribedby?: string; | ||
|
|
||
| className?: string; | ||
| closeOnOverlayClick?: boolean; | ||
| closeOnEscape?: boolean; | ||
| } |
There was a problem hiding this comment.
접근성을 위해 모달 다이얼로그에는 접근 가능한 이름이 반드시 있어야 합니다. 이는 aria-label 또는 aria-labelledby를 통해 제공될 수 있습니다. 현재 ModalProps 인터페이스는 ariaLabel을 항상 문자열로 요구하고 있어 이름이 제공되도록 보장하는 좋은 방법입니다. 하지만 개발자가 모달 내부의 보이는 제목을 가리키기 위해 ariaLabelledby를 사용하려는 경우(이 방법이 종종 더 선호됩니다), 여전히 불필요한 ariaLabel을 제공해야 합니다. ariaLabel 또는 ariaLabelledby 둘 중 하나가 제공되도록 강제하는 것이 더 좋습니다. interface를 type으로 변경하고 유니언 타입을 사용하여 이를 달성할 수 있습니다. 이렇게 하면 컴포넌트의 API가 더 유연해지고 시맨틱적으로 정확해집니다.
| export interface ModalProps { | |
| isOpen: boolean; | |
| onClose: () => void; | |
| children?: ReactNode; | |
| ariaLabel: string; | |
| ariaLabelledby?: string; | |
| ariaDescribedby?: string; | |
| className?: string; | |
| closeOnOverlayClick?: boolean; | |
| closeOnEscape?: boolean; | |
| } | |
| export type ModalProps = { | |
| isOpen: boolean; | |
| onClose: () => void; | |
| children?: ReactNode; | |
| ariaDescribedby?: string; | |
| className?: string; | |
| closeOnOverlayClick?: boolean; | |
| closeOnEscape?: boolean; | |
| } & ( | |
| | { | |
| ariaLabel: string; | |
| ariaLabelledby?: never; | |
| } | |
| | { | |
| ariaLabel?: never; | |
| ariaLabelledby: string; | |
| } | |
| ); |
There was a problem hiding this comment.
import { ReactNode } from 'react';
interface BaseModalProps {
isOpen: boolean;
onClose: () => void;
children?: ReactNode;
ariaDescribedby?: string;
className?: string;
closeOnOverlayClick?: boolean;
closeOnEscape?: boolean;
}
export type ModalProps =
| (BaseModalProps & { ariaLabel: string; ariaLabelledby?: never })
| (BaseModalProps & { ariaLabel?: never; ariaLabelledby: string });코드리뷰 내용을 반영하되 조금 더 좋은 방향으로 개선했습니다.
베이스 인터페이스를 두고, 접근성 관련 타입을 별도로 추가했습니다.
| <section | ||
| className={clsx(style.overlay, className)} | ||
| onClick={closeOnOverlayClick ? onClose : undefined} | ||
| role="presentation" | ||
| > | ||
| <div | ||
| className={style.contentsBox} | ||
| ref={dialogRef} | ||
| onClick={stopPropagation} | ||
| onKeyDown={onKeyDown} | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-label={ariaLabel} | ||
| tabIndex={-1} | ||
| {...ariaProps} | ||
| > | ||
| {children} | ||
| </div> | ||
| </section> |
There was a problem hiding this comment.
오버레이 요소로 role="presentation" 속성을 가진 <section>을 사용하고 있습니다. role="presentation"이 <section>의 시맨틱 의미를 제거하므로 기능적으로는 문제가 없지만, 이처럼 순수하게 표현적인 래퍼(wrapper) 역할만 하는 요소에는 <div>를 사용하는 것이 더 일반적이고 코드의 의도를 명확하게 나타냅니다. <section> 요소는 일반적으로 제목(heading)과 함께 콘텐츠의 주제별 그룹을 나타내는 데 사용해야 합니다.
| <section | |
| className={clsx(style.overlay, className)} | |
| onClick={closeOnOverlayClick ? onClose : undefined} | |
| role="presentation" | |
| > | |
| <div | |
| className={style.contentsBox} | |
| ref={dialogRef} | |
| onClick={stopPropagation} | |
| onKeyDown={onKeyDown} | |
| role="dialog" | |
| aria-modal="true" | |
| aria-label={ariaLabel} | |
| tabIndex={-1} | |
| {...ariaProps} | |
| > | |
| {children} | |
| </div> | |
| </section> | |
| <div | |
| className={clsx(style.overlay, className)} | |
| onClick={closeOnOverlayClick ? onClose : undefined} | |
| role="presentation" | |
| > | |
| <div | |
| className={style.contentsBox} | |
| ref={dialogRef} | |
| onClick={stopPropagation} | |
| onKeyDown={onKeyDown} | |
| role="dialog" | |
| aria-modal="true" | |
| aria-label={ariaLabel} | |
| tabIndex={-1} | |
| {...ariaProps} | |
| > | |
| {children} | |
| </div> | |
| </div> |
| import { getFocusableElements } from '../utils/focusable'; | ||
| import type React from 'react'; | ||
|
|
||
| export function useFocusTrap(isOpen: boolean, dialogRef: React.RefObject<HTMLDivElement | null>) { |
There was a problem hiding this comment.
dialogRef의 타입이 React.RefObject<HTMLDivElement | null>로 선언되어 있습니다. useRef<HTMLDivElement>(null)을 사용하여 생성된 ref 객체의 타입은 React.RefObject<HTMLDivElement>입니다. 이 타입의 current 속성이 HTMLDivElement | null이 됩니다. 현재 타입도 호환은 되지만, 혼란을 줄 수 있고 정확하지 않습니다. React.RefObject<HTMLDivElement>로 수정하는 것이 더 명확하고 정확한 타입 선언입니다.
| export function useFocusTrap(isOpen: boolean, dialogRef: React.RefObject<HTMLDivElement | null>) { | |
| export function useFocusTrap(isOpen: boolean, dialogRef: React.RefObject<HTMLDivElement>) { |
- 베이스 타입을 인터페이스로 분리 - 접근성 타입을 추가로 확장
jungwon123
left a comment
jungwon123
left a comment
There was a problem hiding this comment.
코드 작성하시느라 고생하셨습니다! 제미나이 리뷰 확인해보시고 정말 필요로 수정 할 부분만 하면 될거같습니다. 승인은 하겠습니다!
2 participants
Summary
모달 컴포넌트 기본 기능 구현했습니다.
Issue
연관된 이슈 #1