-
Notifications
You must be signed in to change notification settings - Fork 5
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
Changes from 6 commits
9ebbf66
e988844
7355b00
1cecfda
fd6256e
b79987b
8138b11
e0050b9
2b42e12
59e43d2
77ea2ff
2d1bb16
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,6 @@ | |
|
||
import Toss from '@assets/image/Toss_Symbol_Primary.png'; | ||
import InputDelete from '@assets/image/inputDelete.svg'; | ||
import Buljusa from '@assets/image/buljusa.svg'; | ||
import Error from '@assets/image/error.svg'; | ||
import Confirm from '@assets/image/confirm.svg'; | ||
import Trash from '@assets/image/trash.svg'; | ||
|
@@ -20,27 +19,27 @@ import {useTheme} from '@theme/HDesignProvider'; | |
|
||
import {iconStyle} from './Icon.style'; | ||
|
||
const ICON = { | ||
inputDelete: <InputDelete />, | ||
buljusa: <Buljusa />, | ||
rightChevron: <RightChevron />, | ||
search: <Search />, | ||
error: <Error />, | ||
confirm: <Confirm />, | ||
trash: <Trash />, | ||
trashMini: <TrashMini />, | ||
check: <Check />, | ||
x: <X />, | ||
pencilMini: <PencilMini />, | ||
export const ICON = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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에 들어가는 것이 더 적절하지 않나? 입니당. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 아, 그리고 aria-label이 IconButton에 존재하려면 Icon에 있는 onClick 또한 IconButton에 있는 것이 더 적절하겠네용 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 아하 확실히 그럴 수 있겠네요. 소하 의견처럼 고려해도 좋을 것 같아요. 제가 이미지에 라벨을 단 이유는 Icon컴포넌트를 사용하는 곳에서 라벨을 매번 부착하는 수고를 덜기 위함이었습니다. 이미지에 달면 매번 라벨을 써줄 필요가 없어지니까요. 그리고 생각해보니 Icon에 따라 라벨이 나뉘는게 아니라, x버튼이어도 어떤 닫기 버튼인지 이름이 다를 수 있을 것 같아 매번 라벨을 사용하는 곳에서 지정해주는게 맞지 않을까 싶기도 해요. 그래서 위 사항들을 고려해 제가 생각한 방법은 ICON에 적용한 모든 라벨을 제거하고 그냥 라벨을 사용해야하는 사용처에서 IconButton에 라벨을 달아주는 것이 어떠한가.. 입니다! 어떤가요?! 번거롭긴 하겠지만 의미를 분명히 전달하는게 중요한 낭독기 사용자들을 위해선 필요한 번거로움이지 않을까 싶어요. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 저도 동의합니다! 같은 아이콘이어도 다른 aria-label을 주는 상황이 생길 수도 있으니깐요! |
||
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 />, | ||
editPencil: <EditPencil />, | ||
heundeut: <img src={`${process.env.IMAGE_URL}/heundeut.svg`} />, | ||
photoButton: <PhotoButton />, | ||
}; | ||
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; | ||
|
||
export const Icon: React.FC<IconProps> = ({iconColor, iconType, ...htmlProps}: IconProps) => { | ||
export const Icon = ({iconColor, iconType, ...htmlProps}: IconProps) => { | ||
const {theme} = useTheme(); | ||
|
||
return ( | ||
<div css={iconStyle({iconType, theme, iconColor})} {...htmlProps}> | ||
{ICON[iconType]} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,23 +1,9 @@ | ||
import {Theme} from '@theme/theme.type'; | ||
import {ColorKeys} from '@token/colors'; | ||
|
||
export type IconType = | ||
| 'inputDelete' | ||
| 'buljusa' | ||
| 'rightChevron' | ||
| 'search' | ||
| 'error' | ||
| 'confirm' | ||
| 'trash' | ||
| 'trashMini' | ||
| 'check' | ||
| 'x' | ||
| 'pencilMini' | ||
| 'toss' | ||
| 'meatballs' | ||
| 'editPencil' | ||
| 'heundeut' | ||
| 'photoButton'; | ||
import {ICON} from './Icon'; | ||
|
||
export type IconType = keyof typeof ICON; | ||
Comment on lines
+4
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 와 이거 매번 추가해주기 귀찮았는데~~ 너무 좋아요 |
||
|
||
export type IconColor = ColorKeys; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ export const IconButton: React.FC<IconButtonProps> = forwardRef<HTMLButtonElemen | |
ref, | ||
) { | ||
const {theme} = useTheme(); | ||
|
||
return ( | ||
<button ref={ref} css={iconButtonStyle({theme, size, variants})} {...htmlProps}> | ||
{children} | ||
Comment on lines
11
to
17
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 맞아요! 일단 제가 코드상에서 의미를 확인할 수 있는 IconButton에 모두 라벨을 달아볼게요!~! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
onClick이 Icon이 아닌 IconButton에 있는 것이 더 적절해 보입니다!
현재 프로젝트 내에서 IconButton과 Icon에 onClick을 사용한다면 IconButton에 있기도 하구요.
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.
그냥 IconButton으로만 감싸면 해결되었어서 이 부분을 놓쳤군요..!! 바로 반영했습니다~!!