-
-
Notifications
You must be signed in to change notification settings - Fork 3
컴포넌트 접근성 개선(aria-invalid, aria-required) #724
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Bundle ReportChanges will increase total bundle size by 22.63kB (4.08%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: daleui-bundle-esmAssets Changed:
|
c79ed00 to
7dfdb37
Compare
497009a to
27e592b
Compare
@hyoseong1994 동작이 달라도 괜찮다는 말씀이신가요? 추가 검토가 필요하시다는 말씀이신가요? |
| } | ||
|
|
||
| const { tone, disabled: groupDisabled } = context; | ||
| const { tone, disabled: groupDisabled, invalid: GroupInvalid } = context; |
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.
GroupInvalid만 대문자로 시작해야하는 이유가 있을까요?
| const { tone, disabled: groupDisabled, invalid: GroupInvalid } = context; | |
| const { tone, disabled: groupDisabled, invalid: groupInvalid } = context; |
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.
티켓 분리로 인하여 제거되었습니다. 감사합니다.
| className={css({ display: "flex", flexDirection: "column", gap: "32" })} | ||
| > | ||
| <RadioGroup | ||
| {...args} |
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.
나이스 캐치!
| argTypes: { | ||
| name: { | ||
| control: false, | ||
| }, | ||
| label: { | ||
| control: false, | ||
| }, | ||
| orientation: { | ||
| control: false, | ||
| }, | ||
| defaultValue: { | ||
| control: false, | ||
| }, | ||
| }, |
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.
컨벤션 문서에 내용 추가했습니다.
https://github.com/DaleStudy/daleui/wiki/Conventions#%EC%8A%A4%ED%86%A0%EB%A6%AC
항상 고정되어야 하는 prop은 Story의 argTypes에서 control: false로 선언합니다.
ex) Orientation Story에서 orientation props
| /** | ||
| * true이면 이 라디오 버튼이 에러 상태가 됩니다. | ||
| * @default false | ||
| */ | ||
| invalid?: boolean; |
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.
개별 Radio 컴포넌트에 invalid 속성을 추가했는데, 어떤 사용 케이스를 생각하셨는지 궁금합니다. (그리고 컴포넌트의 API를 변경하는 것은 본 티켓의 목표에서 벗어날 수 있겠다는 생각도 드네요.)
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.
사실 사용케이스를 고민한것은 아니고 figma에 디자인이 되어있는데 해당 기능이 없길래 누락된건가 싶어서 추가했습니다.
개별 라디오에 대해서는 invalid가 필요하지 않겠네요 사용케이스가 없을거같습니다.
API 변경에 대해서 동의합니다. 후속 티켓 생성하여 작업 분리하도록하겠습니다.
| expect(inputElement).not.toHaveAttribute("aria-required"); | ||
| }); | ||
|
|
||
| it("leadingIcon과 trailingIcon이 제공될 때 올바르게 렌더링되어야 합니다.", () => { |
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.
아직도 it 친구가 있었구나!
|
@DaleSeo 안녕하세요 달레님
동작이 달라서 여기만 왜 다른지에 대한 질문이 있을거같아서 작성해둔내용입니다.
|
bd06ec5 to
4aae3ec
Compare
변경 사항
목적
리뷰어에게
PR 작성자 체크 리스트