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] 접근성 개선 : 지출내역 추가 Flow의 Top #758

Merged
merged 2 commits into from
Oct 17, 2024
Merged

Conversation

soi-ha
Copy link
Contributor

@soi-ha soi-ha commented Oct 16, 2024

issue

구현 목적

핵심 flow인 지출내역 생성에서 스크린리더를 사용하는 유저는 header를 제외하고는 가장 먼저 Top을 듣게 될 것입니다. Top 컴포넌트는 해당 페이지에 대한 설명을 담은 컴포넌트로, 스크린리더 사용자는 해당 페이지에서 어떤 것을 입력해야 하는지 듣게 됩니다.

그런데, 기존의 Top 컴포넌트의 text는 끊어서 읽혔습니다. 이런 점은 스크린리더 사용자에게 혼동과 불편함을 주게 될 것입니다. 따라서, Top 컴포넌트를 스크린리더가 읽혔을 때 내용이 한번에 읽히도록 수정합니다.

개선 전/후 영상

🔇 영상 속 스크린리더기는 음성을 사용하지 않습니다!
💬 영상 하단에 스크린리더 자막으로 확인해주세요!

  • 개선 전

  • ScreenRecording_10-16-2024.17-49-28_1.mov
  • 개선 후

    ScreenRecording_10-16-2024.17-28-06_1.mov

구현 방법

const [childrenTexts, setChildrenTexts] = useState<string[]>([]);

  useEffect(() => {
    const collectedTexts: string[] = [];
    React.Children.forEach(children, child => {
      if (React.isValidElement(child) && child.type === Top.Line) {
        collectedTexts.push(child.props.text);
      }
    });
    setChildrenTexts(collectedTexts);
  }, [children]);

  return (
    <div
      css={css`
        display: flex;
        flex-direction: column;
      `}
      aria-label={childrenTexts.join(' ')}
      tabIndex={0}
    >
      {children}
    </div>
  );
  • childrenTexts 상태 생성

  • Top 컴포넌트의 children(즉, Line 컴포넌트)를 map합니다.

    이때, children의 props가 text인 값을 collectedTexts에 저장합니다.
    이 collectedTexts의 값을 setChildrenTexts를 통해 상태를 업데이트 합니다.

  • Top 컴포넌트의 aria-label로 childrenTexts를 넣습니다.

    이때, childrenTexts를 배열이라 join을 사용하여 하나의 문자열로 만듭니다.

  • tabIndex의 값을 0을 주어, Top 컴포넌트만 상위로 읽히도록 합니다.

export default function Line({text, emphasize = []}: Props) {
  // ...
  
  {elements.map((text, index) => {
    return (
      <Text
        key={`${text}-${index}`}
        size="subTitle"
        textColor={emphasize.includes(text) ? 'black' : 'gray'}
        style={{whiteSpace: 'pre'}}
        aria-hidden={true}
      >
        {`${text}`}
      </Text>
    );
  })}
}
  • 이때, Line 컴포넌트에 aria-hidden=true를 추가합니다.

    Top 컴포넌트 이후에 Line 컴포넌트가 같이 스크린리더로 읽히기 때문입니다.

@soi-ha soi-ha added 🖥️ FE Frontend 🚧 refactor refactoring labels Oct 16, 2024
@soi-ha soi-ha added this to the v2.1.1 milestone Oct 16, 2024
@soi-ha soi-ha self-assigned this Oct 16, 2024
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.

안녕하세요 소하~!
생각하지 못했던 방법으로 어려운 문제를 간단하게 해결 한 것 같아서 신기하고 대단하네요 ㅋㅋㅋ
다만, 한가지 궁금한게 있어서 변경요청드려요. 제가 봤을 땐, useState, useEffect를 사용하지 않아도, 계산 가능한 const 변수에 childrenText를 담아도 children이 변경됐을 때, Top component가 재렌더 되면서 정상적으로 작동할 수 있을 것 같아요.
한번 확인해 보시고 회신주세용~!
고생 많았습니당

Comment on lines 13 to 23
const [childrenTexts, setChildrenTexts] = useState<string[]>([]);

useEffect(() => {
const collectedTexts: string[] = [];
React.Children.forEach(children, child => {
if (React.isValidElement(child) && child.type === Top.Line) {
collectedTexts.push(child.props.text);
}
});
setChildrenTexts(collectedTexts);
}, [children]);
Copy link
Contributor

Choose a reason for hiding this comment

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

  const childrenTexts = React.Children.toArray(children)
    .filter((child): child is React.ReactElement => React.isValidElement(child) && child.type === Top.Line)
    .map(child => child.props.text);

state와 useEffect를 사용하지 않고도 가능하지 않을까 싶어요~!

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 Author

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.

Top을 한 번에 읽을 수 있게 되어서 시각적 제한이 있는 사용자가 헷갈리지 않게 Top 문구를 읽을 수 있을 것 같아요.

Approve는 하지만 제가 생각했을 때 개선 사항이 있습니다! 코멘트 확인해주세요

Comment on lines 13 to 24
const [childrenTexts, setChildrenTexts] = useState<string[]>([]);

useEffect(() => {
const collectedTexts: string[] = [];
React.Children.forEach(children, child => {
if (React.isValidElement(child) && child.type === Top.Line) {
collectedTexts.push(child.props.text);
}
});
setChildrenTexts(collectedTexts);
}, [children]);

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [childrenTexts, setChildrenTexts] = useState<string[]>([]);
useEffect(() => {
const collectedTexts: string[] = [];
React.Children.forEach(children, child => {
if (React.isValidElement(child) && child.type === Top.Line) {
collectedTexts.push(child.props.text);
}
});
setChildrenTexts(collectedTexts);
}, [children]);
const collectedTexts: string[] = [];
React.Children.forEach(children, child => {
if (React.isValidElement(child) && child.type === Top.Line) {
collectedTexts.push(child.props.text);
}
});

useEffect를 사용하지 않고 이렇게 바로 children의 파생 상태를 이용할 수 있을 것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오 ㅋㅋㅋㅋㅋ 토다리랑 동일한 부분을 말씀해주셨네요! 꼭 반영해보겠습니당

return (
<div
css={css`
display: flex;
flex-direction: column;
`}
aria-label={childrenTexts.join(' ')}
tabIndex={0}
Copy link
Contributor

Choose a reason for hiding this comment

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

해당 div에 tab focus를 주기 위해 tabIndex=0을 주신 건가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

맞습니당! tabIndex가 존재하지 않으면 Top 컴포넌트 전체에 스크린리더가 포커스가 안 가더라구요

Copy link
Contributor

@pakxe pakxe left a comment

Choose a reason for hiding this comment

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

Top > Line구조이고, Top컴포넌트에서 모든 텍스트를 읽히도록 하고 Line에는 포커스가 가지 않도록 aria-hidden을 준게 맞을까요? 제가 이해한게 맞다면 깔끔하게 개선해주신 것 같다 생각해요~!.!

그리고 상태를 사용하지 않고 구현하는 커멘트에 대해선, 이미 다른 리뷰어가 RC를 주었기 때문에 저는 어쁘로브하도록 할게요! 😆

(p.s. 개선 전/후 영상 순서가 바뀐 것 같습니다...!)

Comment on lines 13 to 23
const [childrenTexts, setChildrenTexts] = useState<string[]>([]);

useEffect(() => {
const collectedTexts: string[] = [];
React.Children.forEach(children, child => {
if (React.isValidElement(child) && child.type === Top.Line) {
collectedTexts.push(child.props.text);
}
});
setChildrenTexts(collectedTexts);
}, [children]);
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

@soi-ha
Copy link
Contributor Author

soi-ha commented Oct 17, 2024

Top.Line에서 사용하고 있는 texts를 가져올 때, 기존에는 useState와 useEffect를 사용했습니다.
PR 리뷰를 통해 두 hook을 사용하지 않고도 충분히 기능 구현이 가능하여 다음과 같이 수정해줬습니다. 1cb6776

리뷰에서 토다리와 쿠키가 각자의 방식을 공유해줬습니다. 둘 중에서 쿠키의 방식을 차용하게 되었습니다.
이때, 효율성 측면을 최우선으로 고려했습니다. toArray를 사용하는 방식은 배열을 두번 순환하게 됩니다. 빈 배열을 만들고 map을 하는 방식은 배열을 한번만 순환합니다.
그렇기에 빈 배열과 map을 사용하는 방식 (기존방식에서 hook만 제거한 방식)으로 리팩토링했습니다.

@soi-ha soi-ha requested a review from Todari October 17, 2024 04:41
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.

LGTM

@Todari Todari merged commit 27a8d88 into fe-dev Oct 17, 2024
2 checks passed
@Todari Todari deleted the feature/#757 branch October 17, 2024 06:27
@Todari Todari mentioned this pull request Oct 17, 2024
Todari pushed a commit that referenced this pull request Oct 17, 2024
* fix: Top 컴포넌트 접근성 개선

* fix: 불필요한 useState와 useEffect를 제거
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🖥️ FE Frontend 🚧 refactor refactoring
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants