-
Notifications
You must be signed in to change notification settings - Fork 2
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
[FE] feat: 리뷰 작성 페이지 모달 접근성 개선 #906
Conversation
…eview-me into fe/feat/873-modal-a11y
setHasAnnounced(true); | ||
setTimeout(() => { | ||
setHasAnnounced(false); | ||
}, 200); |
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.
setTimeout의 시간을 0초로 설정해서 즉각적으로 상태를 변화시키는 경우, 스크린 리더가 인식하지 못하는 문제가 있어 약간의 딜레이를 주었습니다.
// 모달을 포함하지 않는 body의 자식들에 aria-hidden 적용 | ||
const bodyChildren = document.body.children; | ||
for (const bodyChildrenElement of bodyChildren) { | ||
if (!bodyChildrenElement.contains(focusTrapRef.current)) { | ||
bodyChildrenElement.setAttribute('aria-hidden', 'true'); | ||
} | ||
} |
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.
모달을 포함하는 상위 요소(portal)에는 aria-hidden
속성이 적용되면 안 되기 때문에 이렇게 처리했습니다.
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.
이 친절한 코멘트들이 코드 주석으로 들어가있으면 나중에 코드만 확인할 때 더 편할 것 같습니다!
'a[href], button, textarea, input, select, [tabindex]:not([tabindex="-1"])', | ||
) || [], | ||
).filter((element) => { | ||
const el = element as HTMLElement; |
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.
Aarray.from 이 빈 배열이면 el는 undefined가 되는데 as로 타입을 강제하기 보다, filter이전에 빈 배열을 거르거나, 타입 내로잉으로 el가 HTMLElement가 아닌 것을 거르지 않은 이유가 있나요?
lastElement.focus(); | ||
event.preventDefault(); | ||
} | ||
} else { |
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.
여러 if문이 중첩되어있어서, early return을 사용해보면 어떨까요?
const [hasAnnounced, setHasAnnounced] = useState(false); | ||
const focusTrapRef = useRef<HTMLDivElement>(null); | ||
|
||
useEffect(() => { |
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.
useEffect 안의 코드가 상당히 기네요. 😮
코드가 기니까, 컴포넌트 마운트, 언마운트 시 무엇을 할 것인지 파악하는데 시간이 걸려요.
}, []); | ||
|
||
return ( | ||
<S.FocusTrapContainer ref={focusTrapRef} role="dialog" aria-modal={true}> |
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.
컴포넌트로 감싸는 거 좋네요!
@@ -28,9 +28,31 @@ const AnswerListRecheckModal = ({ questionSectionList, answerMap, closeModal }: | |||
return answer ? answer.text : ''; | |||
}; | |||
|
|||
const generateSummary = () => { |
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.
선택한 답변만 읽어주니까 스크린 리더기 사용자가 작성한 답변을 확인하는데 굉장히 편리할 것 같아요. 👍✨
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.
tab, 스크린 리더기 모두 확인했어요. 문제 없고 좋네요! 👍✨
코드 관련해서 가독성을 위해 리팩토링 시 생각해보면 좋을 것들이 있어서 코멘트 남겼어요.
런칭이 얼마 남지 않아, 리팩토링은 나중에 해도 된다고 생각해 approve 할게요
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.
사소한 사항들 코멘트로 남겼어요~~~
document.addEventListener('focusin', handleFocusIn); | ||
|
||
// 모달을 포함하지 않는 body의 자식들에 aria-hidden 적용 | ||
const bodyChildren = document.body.children; |
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.
body.children의 리턴값이 HTMLCollection이니까 변수를 복수형으로 바꾸면 좋을 것 같아용
// 모달을 포함하지 않는 body의 자식들에 aria-hidden 적용 | ||
const bodyChildren = document.body.children; | ||
for (const bodyChildrenElement of bodyChildren) { | ||
if (!bodyChildrenElement.contains(focusTrapRef.current)) { | ||
bodyChildrenElement.setAttribute('aria-hidden', 'true'); | ||
} | ||
} |
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.
이 친절한 코멘트들이 코드 주석으로 들어가있으면 나중에 코드만 확인할 때 더 편할 것 같습니다!
<S.FocusTrapContainer ref={focusTrapRef} role="dialog" aria-modal={true}> | ||
{hasAnnounced && ( | ||
<span aria-live="assertive" className="sr-only"> | ||
모달이 열렸습니다 |
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.
모달 이름을 받아서 어떤 모달이 열렸는지를 설명해주면 좋을 것 같아요!
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.
코멘트는 나중에 천천히 반영해주세요~~ 포커스 로직이 생각보다 많이 들어가 있어서 어려웠을 텐데 금방 구현해주셨네요!
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.
빠른 approve 할게요!! 추후에 코드 리뷰 진행할게요!
수고했어요 에프이!
* design: 접근성 컴포넌트 전용 스타일 추가 * feat: 키보드 탭과 스크린 리더의 포커스가 모달을 벗어나지 않도록 제한하는 포커스 트랩 구현 * chore: 각 공통 모달 컴포넌트에 포커스 트랩 적용 * feat: 제출 전 확인 모달에서 사용자가 작성한 내용만을 요약해서 읽어주는 기능 구현
🚀 어떤 기능을 구현했나요 ?
FocusTrap
을 구현했습니다.🔥 어떻게 해결했나요 ?
📝 어떤 부분에 집중해서 리뷰해야 할까요?
📚 참고 자료, 할 말