Skip to content

[항공사 웹사이트의 컴포넌트 접근성 높이기 - 1단계] 비녀(강윤호) 미션 제출합니다. #52

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 16 commits into from
Oct 11, 2022
Merged

[항공사 웹사이트의 컴포넌트 접근성 높이기 - 1단계] 비녀(강윤호) 미션 제출합니다. #52

merged 16 commits into from
Oct 11, 2022

Conversation

KangYunHo1221
Copy link

@KangYunHo1221 KangYunHo1221 commented Oct 6, 2022 •
edited
Loading

🔥 결과

직접 구현한 페이지를 작동시켜볼 수 있도록 접근 가능한 페이지 링크를 공유해주세요.

링크

✅ 요구사항 목록

1 Spin Button

  • 최대 인원수는 3명까지만 가능하게 구현합니다.
  • 실제 스크린 리더는 아래와 같이 읽을 수 있어야 합니다. 단, 스크린 리더기 마다 읽는 방법이 다를 수 있으니 참고만 해주세요. 중요한건 스크린 리더기를 활용하여 동작을 할 수 있어야 합니다.
  • 리팩터링 과정에서 불필요하거나 웹 표준에 어긋나는 마크업은 없는지 확인합니다.
성인 탑승자 한명 줄이기 버튼
성인 1 텍스트 숫자만 수정
성인 탑승자 한명 늘리기 버튼
성인 승객 추가 2
성인 탑승자 한명 늘리기 버튼
성인 승객 추가 3

2 Carousel

  • 총 8개의 carousel 중 2개의 목록을 기본으로 보여준다.
  • 실제 스크린 리더는 아래와 같이 읽을 수 있어야 합니다. 단, 스크린 리더기 마다 읽는 방법이 다를 수 있으니 참고만 해주세요. 중요한건 스크린 리더기를 활용하여 동작을 할 수 있어야 합니다.
  • 리팩터링 과정에서 불필요하거나 웹 표준에 어긋나는 마크업은 없는지 확인합니다.
1. 지금 떠나기 좋은 여행
2. 목록 8개 항목 포함 서울/인천 로스앤젤레스 일반석 왕복 1,481,800 대한민국 원 링크 목록 항목
3. 다음 버튼 (사용 중지)
4. 이전 버튼 (사용 중지)

🧐 공유

/_ 작업하면서 든 생각, 질문, 새롭게 학습하거나 시도해본 내용 등등 공유할 사항이 있다면 자유롭게 적어주세요 _/

  • 윈도우라서 그런지 div로 밑에 넣어준 comment를 못 읽어주는 것 같네요... 혹시 해결방법 아시나요?
  • 툴팁을 넣었습니다. 툴팁도 윈도우에서 잘 못 읽는 이슈가 있어서 조금 변형해서 넣었습니다.
    assertive 와 described by 를 같이 넣었어요.. 괜찮은지 모르겠네요

데모페이지가 뭔가 이상하네요... serve로 local에서 켜도 되고, webpack으로도 development로도 잘 돌아가는데 왜 pages로 하니까 잘 안될까요.. 정 안되면 s3로 해서 배포해드리겠습니다.

@KangYunHo1221 KangYunHo1221 requested a review from kamwoo October 6, 2022 06:21
kamwoo
kamwoo suggested changes Oct 7, 2022
View reviewed changes
Copy link

@kamwoo kamwoo left a comment

Choose a reason for hiding this comment

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

안녕하세요! 비녀 👋
전체적으로 깔끔하고 미션 요구사항도 잘 만족시켰다는 생각이 들었어요~
리뷰하면서 생각하는 것들 코멘트로 남겼습니다!

src/components/Tooltip.tsx
<>
<S.Question aria-describedby='tooltip'>?</S.Question>
<S.StyledTooltip aria-live='assertive' id='tooltip' role='tooltip'>
국민은행 61210204071715로 후원해 결식아동을 도와보세요!
Copy link

@kamwoo kamwoo Oct 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

큰일납니다. 👮‍♂️

src/components/Tooltip.tsx Outdated
const Tooltip = () => {
return (
<>
<S.Question aria-describedby='tooltip'>?</S.Question>
Copy link

Choose a reason for hiding this comment

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

스크린 리더가 "물음표 버튼"이라고 하기 보단 성인에 대한 툴팁이라는 것을 알려줬으면 조금 더 편하지 않을까 생각했어요🤔

Copy link
Author

Choose a reason for hiding this comment

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

aria-label
를 이용해서 tooltip button이라고 읽어주게 변경하였습니다!

src/index.tsx
import { createRoot } from 'react-dom/client';
import App from './App';

const container = document.getElementById('root')!;
Copy link

Choose a reason for hiding this comment

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

오 뒤에 !는 처음보는데 뭐하는 건가요?

Copy link
Author

Choose a reason for hiding this comment

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

무조건 null이나 undefined 아니라는 뜻입니다.
그냥 쓰면 document.getElement return 값이 있다는 것을 명시해줍니다. 없으면 type guard 해줘야해요

image
출처:https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-0.html

tsconfig.json Outdated
"jsx": "react-jsx",
"skipLibCheck": true,
"incremental": true,
"forceConsistentCasingInFileNames": true,
Copy link

@kamwoo kamwoo Oct 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

로컬에서 실행되지 않는 문제가 있었어요. 이 설정을 켜놓으면 윈도우에서는 모르겠지만 파일의 대소문자를 구분하지 않는 시스템인 맥에서는 에러가 발생한다고 합니다.

TypeScript는 실행 중인 파일 시스템의 대소문자 구분 규칙을 따릅니다. 일부 개발자는 대소문자를 구분하는 파일 시스템에서 작업하고 다른 개발자는 그렇지 않은 경우 문제가 될 수 있습니다.
https://www.typescriptlang.org/ko/tsconfig#forceConsistentCasingInFileNames

microsoft/TypeScript/issues 관련 이슈 16875
microsoft/TypeScript/issues 관련 이슈 50544

이 설정 끄니깐 실행되는 걸로 봐서, 같이 작업할 때는 설정을 추가해야하는지 고민이 되네요🤔

Copy link
Author

Choose a reason for hiding this comment

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

아 crlf가 여기서 또 터졌군요.. 설정을 global로 해놓은줄 알았는데 수정하겟습니다

src/Pages/FirstPage.tsx
Comment on lines +45 to +48
<S.Count
type='number'
min={1}
max={3}
Copy link

Choose a reason for hiding this comment

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

해당 속성을 추가했을 때와 하지 않았을 때 동작이 동작이 똑같은데 추가하신 이유가 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

min max를 추가해주면 스크린 리더가 읽어줄때 min max를 알려줍니다.
js로도 제한이 가능하지만 명시해주는게 더 시맨틱하다고 판단했습니다

kamwoo
kamwoo suggested changes Oct 7, 2022
View reviewed changes
Copy link

@kamwoo kamwoo left a comment

Choose a reason for hiding this comment

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

비녀! 리뷰 남겼어요~ 확인해줘요~

kamwoo
kamwoo approved these changes Oct 11, 2022
View reviewed changes
Copy link

@kamwoo kamwoo left a comment

Choose a reason for hiding this comment

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

비녀~ 수정하신거 잘봤습니다! approve할게요~

@kamwoo kamwoo merged commit ab2a8bb into woowacourse:kangyunho1221 Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kamwoo kamwoo kamwoo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KangYunHo1221 @kamwoo