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
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 44 additions & 44 deletions client/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
"@eslint/compat": "^1.1.0",
"@eslint/js": "^9.6.0",
"@jest/types": "^29.6.3",
"@sentry/webpack-plugin": "^2.22.0",
"@sentry/webpack-plugin": "^2.22.5",
"@storybook/addon-essentials": "^8.2.2",
"@storybook/addon-interactions": "^8.2.2",
"@storybook/addon-links": "^8.2.2",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {useTheme} from '@components/Design/theme/HDesignProvider';

import Text from '../Text/Text';
import Icon from '../Icon/Icon';
import IconButton from '../IconButton/IconButton';

import {chipButtonStyle} from './ChipButton.style';

Expand All @@ -16,10 +17,13 @@ interface Props {

const ChipButton = ({color, text, onClick}: Props) => {
const {theme} = useTheme();

return (
<div css={chipButtonStyle({color, theme})}>
<Text textColor="black">{text}</Text>
<Icon iconType="inputDelete" onClick={onClick} />
<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으로만 감싸면 해결되었어서 이 부분을 놓쳤군요..!! 바로 반영했습니다~!!

</div>
);
};
Expand Down
37 changes: 18 additions & 19 deletions client/src/components/Design/components/Icon/Icon.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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 = {
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을 주는 상황이 생길 수도 있으니깐요!

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]}
Expand Down
20 changes: 3 additions & 17 deletions client/src/components/Design/components/Icon/Icon.type.ts
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
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

Choose a reason for hiding this comment

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

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


export type IconColor = ColorKeys;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
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.

고마워요 대 웨디~!

Expand Down
3 changes: 1 addition & 2 deletions client/src/hooks/useToast/useToast.test.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import {render, renderHook, screen, waitFor} from '@testing-library/react';
import {render, screen, waitFor} from '@testing-library/react';
import {act} from 'react';

import ToastContainer from '@components/Toast/ToastContainer';

import {HDesignProvider} from '@HDesign/index';

import {useToast} from './useToast';
import toast from './toast';

const TOAST_MESSAGE = '테스트 메세지에요.';
Expand Down
Loading