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

[FE] 지출 내역 추가 플로우에서 참여 인원의 토큰의 x버튼에 낭독기 커서가 가도록 함 #759

Merged
merged 12 commits into from
Oct 17, 2024

Conversation

pakxe
Copy link
Contributor

@pakxe pakxe commented Oct 16, 2024

issue

개선 전 / 후

🔻 개선 전

default.mp4

🔻 개선 후

default.mp4

구현 목적

현재 낭독기 사용 시 지출 내역 추가 플로우에서 참여 인원의 토큰(ChipButton)의 x버튼에 낭독기 커서가 가지 않습니다.
따라서 낭독기 사용자는 참여 인원을 삭제할 수 없는 상황인데요. 이를 위해 x버튼에 낭독기 커서가 갈 수 있도록 구현합니다.

구현 방법

커서가 가지 않는 이유는 x버튼이 단순한 'div'태그로만 그려지고 있기 때문입니다. x버튼은 말 그대로 '버튼'이기 때문에 button태그가 사용되어야 낭독기 사용자들에게 쉽게 화면을 읽어줄 수 있어요.

x버튼은 Icon이라는 컴포넌트가 사용되고 있는데요. Icon이면서 버튼이기도 해야하는 경우 프로젝트 내에선 IconButton이라는 컴포넌트를 감싸 사용하는 것으로 확인되었습니다.

따라서 IconButton컴포넌트로 Icon컴포넌트를 감싸주었어요.

<IconButton variants="none">
  <Icon iconType="inputDelete" onClick={onClick} />
</IconButton>

이렇게 바꾸면 바라던대로 커서는 향하지만 "버튼"이라고만 읽히게 됩니다.
따라서 해당 버튼이 어떤 버튼인지 의미를 알려주기 위해 아이콘 이미지들에 aria-label을 사용해주었어요.
하지만 이름과 사용처를 확인 후 의미를 추측해서 적은것 뿐, 더 명확하고 좋은 표현이 있을 수 있기 때문에 더 좋은 라벨이 있다면 커멘트 부탁드립니다 ~!

export const ICON = {
  inputDelete: <InputDelete aria-label="지우기" />,
  rightChevron: <RightChevron aria-label="더보기" />,
  search: <Search aria-label="검색" />,
  error: <Error aria-label="오류" />,
  confirm: <Confirm aria-label="확인" />,
  trash: <Trash aria-label="삭제" />,
  trashMini: <TrashMini aria-label="삭제" />,
  check: <Check aria-label="선택" />,
  x: <X aria-label="x" />,
  pencilMini: <PencilMini aria-label="수정" />,
  toss: <img src={Toss} width="16" height="16" alt="toss icon" />,
  meatballs: <Meatballs aria-label="메뉴" />,
  editPencil: <EditPencil aria-label="수정" />,
  heundeut: <img src={`${process.env.IMAGE_URL}/heundeut.svg`} aria-label="행동대장 로고" />,
  photoButton: <PhotoButton aria-label="사진" />,
} as const;

alt가 아닌 aria-label을 사용한 이유는 svg파일에는 alt 속성이 없어서 그렇습니다.
중간에 png가 있긴 한데 한개 뿐입니다. 그래서 오히려 png만 alt로 바꾸는게 의아함을 주지 않을까 싶어 aria-label로 통일했는데요. png가 더 생길 것을 생각하여 alt를 사용하는게 좋을 것 같다~ 라면 바꾸겠습니다!

참고

스토리북의 언어 설정을 바꾸지 않은 이유는, 버튼을.. 붜턴~ 이라고 읽는 것 말고는 차이가 없어서 그냥 그대로 녹화해보았어요.
그래도 스토리북에 한국어 설정하는 방법을 알려주어서 고맙습니다 쿳키~


🛠️ 1차 수정

수정 이유

ICON 객체 안의 모든 이미지에 미리 aria-label을 부착해놓으면 사용처에서 명확한 의미를 담아 읽히도록 하기 어려워요.
그래서 ICON 객체 안에 적었던 aria-label을 전부 제거하고, IconButton에 aria-label을 달아서 사용하는 곳에 따라 더 명확한 의미를 전달해줄 수 있도록 했습니다.

구현 내용

일단 아래와 같은 IconButton인 경우 aria-label을 부착해주었는데요.

  • 현재 배포 버전에서 사용되고 있는 컴포넌트인 경우
  • 코드상에서 보기에 어떤 aria-label을 써야하는지 분명하게 알 수 있는 경우

혹시 제가 부착한 aria-label이 다른 내용이 더 적절할 것 같다~ 싶은게 있다면 커멘트 남겨주세요!

@pakxe pakxe added this to the v2.1.1 milestone Oct 16, 2024
@pakxe pakxe requested review from soi-ha, jinhokim98 and Todari October 16, 2024 13:51
@pakxe pakxe self-assigned this Oct 16, 2024
Copy link

Copy link
Contributor

@soi-ha soi-ha left a comment

Choose a reason for hiding this comment

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

웨디가 작성해준 코드와 PR 내용 잘 읽어봤습니다! 개선 후 영상을 보니 이제는 스크린리더로 인원 삭제도 가능해졌네용 👍

다만, 논의해보고 싶은 부분이 있어서 RC했습니당! 확인해보시고 말씀해주세용~

+) 추가로 현재 test에서 오류가 발생하고 있네요!

check: <Check />,
x: <X />,
pencilMini: <PencilMini />,
export const ICON = {
Copy link
Contributor

Choose a reason for hiding this comment

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

aria-label이 Icon에만 들어가게 되는 것일까요?? 수정해주신 코드를 보면 IconButton을 사용하던데 aria-label이 IconButton에도 들어가는게 올바르지 않나 싶어서요!

말 그대로 Icon 컴포넌트는 이미지라고 생각해서 꾸밈을 위한 img 태그에는 alt를 빈문자열을 넣듯이 Icon 컴포넌트에도 aria-label이 존재하지 않아도 된다고 생각이 듭니다!

정리, aria-label이 Icon보다는 IconButton에 들어가는 것이 더 적절하지 않나? 입니당.

Copy link
Contributor

Choose a reason for hiding this comment

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

아, 그리고 aria-label이 IconButton에 존재하려면 Icon에 있는 onClick 또한 IconButton에 있는 것이 더 적절하겠네용

Copy link
Contributor Author

@pakxe pakxe Oct 16, 2024

Choose a reason for hiding this comment

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

아하 확실히 그럴 수 있겠네요. 소하 의견처럼 고려해도 좋을 것 같아요.

제가 이미지에 라벨을 단 이유는 Icon컴포넌트를 사용하는 곳에서 라벨을 매번 부착하는 수고를 덜기 위함이었습니다. 이미지에 달면 매번 라벨을 써줄 필요가 없어지니까요.

그리고 생각해보니 Icon에 따라 라벨이 나뉘는게 아니라, x버튼이어도 어떤 닫기 버튼인지 이름이 다를 수 있을 것 같아 매번 라벨을 사용하는 곳에서 지정해주는게 맞지 않을까 싶기도 해요.

그래서 위 사항들을 고려해 제가 생각한 방법은 ICON에 적용한 모든 라벨을 제거하고 그냥 라벨을 사용해야하는 사용처에서 IconButton에 라벨을 달아주는 것이 어떠한가.. 입니다! 어떤가요?! 번거롭긴 하겠지만 의미를 분명히 전달하는게 중요한 낭독기 사용자들을 위해선 필요한 번거로움이지 않을까 싶어요.

Copy link
Contributor

Choose a reason for hiding this comment

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

소하 의견에 동의합니다~!~! 두분의 생산적인 토론 보기 좋네요 껄껄
저도 코드를 처음 보고 든 생각이, IconButton을 사용하는 사용처에서 label을 attribute로 넘겨주는게 좋을 것 같아용

Copy link
Contributor

Choose a reason for hiding this comment

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

저도 동의합니다! 같은 아이콘이어도 다른 aria-label을 주는 상황이 생길 수도 있으니깐요!

Comment on lines 24 to 26
<IconButton variants="none">
<Icon iconType="inputDelete" onClick={onClick} />
</IconButton>
Copy link
Contributor

Choose a reason for hiding this comment

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

onClick이 Icon이 아닌 IconButton에 있는 것이 더 적절해 보입니다!
현재 프로젝트 내에서 IconButton과 Icon에 onClick을 사용한다면 IconButton에 있기도 하구요.

<IconButton variants="none" onClick={onDelete} style={{display: 'flex', alignItems: 'flex-start'}}>
  <Icon iconType="x" />
</IconButton>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그냥 IconButton으로만 감싸면 해결되었어서 이 부분을 놓쳤군요..!! 바로 반영했습니다~!!

Copy link

Copy link
Contributor

@Todari Todari left a comment

Choose a reason for hiding this comment

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

꼼꼼쓰 웨디의 코드 잘 보고 갑니당~!~!
결국 사용처에서 어떻게 사용하냐가 문제였네요 :)

check: <Check />,
x: <X />,
pencilMini: <PencilMini />,
export const ICON = {
Copy link
Contributor

Choose a reason for hiding this comment

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

소하 의견에 동의합니다~!~! 두분의 생산적인 토론 보기 좋네요 껄껄
저도 코드를 처음 보고 든 생각이, IconButton을 사용하는 사용처에서 label을 attribute로 넘겨주는게 좋을 것 같아용

Comment on lines +4 to +6
import {ICON} from './Icon';

export type IconType = keyof typeof ICON;
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀🚀🚀🚀🚀

Comment on lines 11 to 17
ref,
) {
const {theme} = useTheme();

return (
<button ref={ref} css={iconButtonStyle({theme, size, variants})} {...htmlProps}>
{children}
Copy link
Contributor

Choose a reason for hiding this comment

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

IconButtonProps 에 이미 React.ComponentProps<'button'>을 prop으로 받고 있어서,
현재에도 사용처에서 aria-label=~~~과 같은 속성을 넘겨줄 수 있을거에요.
사용처에서 사용만 한다면 해결되는 문제일거라고 보여지네요!

Copy link
Contributor Author

@pakxe pakxe Oct 17, 2024

Choose a reason for hiding this comment

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

맞아요! 일단 제가 코드상에서 의미를 확인할 수 있는 IconButton에 모두 라벨을 달아볼게요!~!

Copy link
Contributor

Choose a reason for hiding this comment

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

고마워요 대 웨디~!

Copy link
Contributor

@jinhokim98 jinhokim98 left a comment

Choose a reason for hiding this comment

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

다른 프엔분들이 너무 코멘트를 잘 달아줘서 (든든) 저는 approve할게요!
고생했습니당

check: <Check />,
x: <X />,
pencilMini: <PencilMini />,
export const ICON = {
Copy link
Contributor

Choose a reason for hiding this comment

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

저도 동의합니다! 같은 아이콘이어도 다른 aria-label을 주는 상황이 생길 수도 있으니깐요!

| 'photoButton';
import {ICON} from './Icon';

export type IconType = keyof typeof ICON;
Copy link
Contributor

Choose a reason for hiding this comment

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

와 이거 매번 추가해주기 귀찮았는데~~ 너무 좋아요

Copy link

@pakxe
Copy link
Contributor Author

pakxe commented Oct 17, 2024

피드백 주신 내용을 반영하여 코드를 수정해보았습니다!

자세한 수정 내용은 pr본문에 연장하여 작성해두었으니 확인 부탁드려요~~!

@pakxe pakxe requested a review from soi-ha October 17, 2024 02:11
Copy link
Contributor

@Todari Todari left a comment

Choose a reason for hiding this comment

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

사용되는 곳들에서 꼼꼼하게 aria-label을 달아 주셨네요!
섬세퀸 웨디답게 다들 적절하고 너무 좋은 문구로 달아주신 것 같아요 :)
빠른 반영 감사합니다 고생 많았어요!!!

@@ -83,6 +83,7 @@ const Member = ({member, changeMemberName, handleDeleteMember, toggleDepositStat
variants="tertiary"
css={{width: '23px', height: '23px', borderRadius: '0.375rem'}}
onClick={() => handleDeleteMember(member.memberId)}
aria-label="인원 삭제"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -20,7 +20,7 @@ const Nav = ({trackStartCreateEvent}: NavProps) => {
<header style={{display: 'flex', justifyContent: 'space-between', alignItems: 'center', height: '37px'}}>
<TopNav>
<TopNav.Item routePath="/">
<IconButton variants="none">
<IconButton variants="none" aria-label="행동대장 로고">
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -70,12 +70,12 @@ export const Input: React.FC<InputProps> = forwardRef<HTMLInputElement, InputPro
{...htmlProps}
/>
{onDelete && value && hasFocus && (
<IconButton tabIndex={-1} variants="none" onMouseDown={onDelete}>
<IconButton tabIndex={-1} variants="none" onMouseDown={onDelete} aria-label="입력 내용 모두 지우기">
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -20,7 +20,7 @@ const MeatballBase = ({isOpen, setIsOpen, dropdownRef, children}: MeatballBasePr

return (
<>
<IconButton variants="none" onClick={() => setIsOpen(true)}>
<IconButton variants="none" onClick={() => setIsOpen(true)} aria-label="더보기">
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@soi-ha soi-ha left a comment

Choose a reason for hiding this comment

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

반영해준거 잘 봤어요 웨디! 빠르게 반영해주시고.. 사용하지 않는 파일 제거까지 아주 굳입니댜!!

<IconButton variants="none" onClick={onDelete} style={{display: 'flex', alignItems: 'flex-start'}}>
<IconButton
variants="none"
onClick={onDelete}
Copy link
Contributor

Choose a reason for hiding this comment

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

반영 최고에용!

Comment on lines +24 to +26
<IconButton variants="none" onClick={onClick} aria-label="인원 지우기">
<Icon iconType="inputDelete" />
</IconButton>
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@Todari Todari merged commit 1843235 into fe-dev Oct 17, 2024
2 checks passed
@Todari Todari deleted the feature/#747 branch October 17, 2024 06:29
@Todari Todari mentioned this pull request Oct 17, 2024
Todari pushed a commit that referenced this pull request Oct 17, 2024
* chore: @sentry/webpack-plugin 라이브러리 2.22.0 -> 2.22.5로 업데이트

* feat: Icon 컴포넌트를 IconButton으로 감싸 버튼으로 인식되도록 함

* feat: ICON으로 사용되는 모든 이미지에 aria-label 부착

* feat: 하드코딩된 IconType의 타입을 ICON 객체의 프로퍼티 키 값으로 하도록 수정

* style: 코드 컨벤션에 맞도록 수정

* chore: 사용되지 않는 import 제거

* chore: 사용하지 않는 컴포넌트 제거

* feat: onClick을 Icon에서 IconButton으로 이동

* chore: 사용하지 않는 컴포넌트 제거

* chore: 사용하지 않는 컴포넌트 제거

* feat: ICON에 달아준 aria-label을 모두 제거

* feat: IconButton 컴포넌트를 사용하는 대부분의 위치에 aria-label 속성 추가
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants