-
Notifications
You must be signed in to change notification settings - Fork 4
Refactor(Callout): 컴포넌트 문서화 및 스타일 개선 #320
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?
Changes from all commits
ff8f64b
2a99a31
a72fb13
e6b7c8e
894458a
4b5da41
aeb722f
098b26e
08f4619
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,73 +1,146 @@ | ||
| import { Callout } from "@sopt-makers/ui"; | ||
| import { Meta, StoryObj } from "@storybook/react"; | ||
| import { Callout } from '@sopt-makers/ui'; | ||
| import { Meta, StoryObj } from '@storybook/react'; | ||
| import type React from 'react'; | ||
|
|
||
| interface CalloutProps { | ||
| children: React.ReactNode; | ||
| type: "danger" | "information" | "warning"; | ||
| hasIcon?: boolean; | ||
| buttonLabel?: string; | ||
| isButtonDisabled?: boolean; | ||
| onClick?: () => void; | ||
| } | ||
| type CalloutProps = React.ComponentProps<typeof Callout>; | ||
|
|
||
| export default { | ||
| title: "Components/Callout", | ||
| title: 'Components/Callout', | ||
| component: Callout, | ||
| tags: ["autodocs"], | ||
| tags: ['autodocs'], | ||
| argTypes: { | ||
| type: { control: 'radio', options: ['danger', 'information', 'warning'] }, | ||
| } | ||
| children: { | ||
| description: '콜아웃의 내용을 작성합니다.', | ||
| control: 'text', | ||
| table: { | ||
| type: { summary: 'ReactNode' }, | ||
| }, | ||
| }, | ||
| type: { | ||
| description: '콜아웃의 타입을 지정합니다. 타입에 따라 색상과 아이콘이 변경됩니다.', | ||
| control: 'radio', | ||
| options: ['danger', 'information', 'warning'], | ||
| table: { | ||
| type: { summary: 'danger | information | warning' }, | ||
| }, | ||
| }, | ||
| hasIcon: { | ||
| description: '아이콘 표시 여부를 지정합니다. buttonLabel이 있으면 이 옵션과 무관하게 아이콘이 항상 표시됩니다.', | ||
| control: 'boolean', | ||
| table: { | ||
| type: { summary: 'boolean' }, | ||
| defaultValue: { summary: 'true' }, | ||
| }, | ||
| }, | ||
| buttonLabel: { | ||
| description: '버튼의 텍스트를 지정합니다. 지정하면 버튼이 표시됩니다.', | ||
| control: 'text', | ||
| table: { | ||
| type: { summary: 'string' }, | ||
| }, | ||
| }, | ||
| isButtonDisabled: { | ||
| description: '버튼의 비활성화 상태를 지정합니다.', | ||
| control: 'boolean', | ||
| table: { | ||
| type: { summary: 'boolean' }, | ||
| defaultValue: { summary: 'false' }, | ||
| }, | ||
| }, | ||
| onClick: { | ||
| description: '버튼 클릭 시 실행할 함수를 지정합니다.', | ||
| action: 'clicked', | ||
| table: { | ||
| type: { summary: '() => void' }, | ||
| }, | ||
| }, | ||
| }, | ||
| } as Meta<CalloutProps>; | ||
|
|
||
| const content = ( | ||
| <> | ||
| hasIcon 옵션으로 통해 아이콘을 표시할 수 있으며 <br /> | ||
| buttonLabel과 onClick 옵션을 통해 버튼의 text와 클릭 핸들러를 설정할 수 있어요 | ||
| </> | ||
| ); | ||
| // danger 콜아웃 스토리 | ||
| export const Danger: StoryObj<CalloutProps> = { | ||
| args: { | ||
| children: "hasIcon 옵션으로 통해 아이콘을 표시할 수 있어요", | ||
| type: "danger", | ||
| children: content, | ||
| type: 'danger', | ||
| hasIcon: false, | ||
| }, | ||
| }; | ||
| // information 콜아웃 스토리 | ||
| export const Information: StoryObj<CalloutProps> = { | ||
| args: { | ||
| children: "hasIcon 옵션으로 통해 아이콘을 표시할 수 있어요", | ||
| type: "information", | ||
| children: 'hasIcon 옵션으로 통해 아이콘을 표시할 수 있으며 ', | ||
| type: 'information', | ||
| hasIcon: false, | ||
| }, | ||
| }; | ||
| // warning 콜아웃 스토리 | ||
| export const Warning: StoryObj<CalloutProps> = { | ||
| args: { | ||
| children: "hasIcon 옵션으로 통해 아이콘을 표시할 수 있어요", | ||
| type: "warning", | ||
| children: 'buttonLabel과 onClick 옵션을 통해 버튼의 text와 클릭 핸들러를 설정할 수 있어요', | ||
| type: 'warning', | ||
| hasIcon: false, | ||
| }, | ||
| }; | ||
| // danger+icon 콜아웃 스토리 | ||
| export const MultipleIconCallouts: StoryObj<CalloutProps> = { | ||
| render: () => ( | ||
| <div style={{ display: 'flex', flexDirection: 'column', gap: '16px' }}> | ||
| <Callout {...(Danger.args as CalloutProps)} hasIcon /> | ||
| <Callout {...(Information.args as CalloutProps)} hasIcon /> | ||
| <Callout {...(Warning.args as CalloutProps)} hasIcon /> | ||
| </div> | ||
| ), | ||
| }; | ||
|
|
||
| // warning+icon+button 콜아웃 스토리 | ||
| export const CalloutWithBtn: StoryObj<CalloutProps> = { | ||
| export const MultipleButtonCallouts: StoryObj<CalloutProps> = { | ||
| render: () => ( | ||
| <div style={{ display: 'flex', flexDirection: 'column', gap: '16px' }}> | ||
| <Callout {...(Danger.args as CalloutProps)} buttonLabel='hover, press 해보세요!'> | ||
| <> | ||
| 버튼이 있는 경우 hasIcon과 무관하게 아이콘이 항상 표시돼요. <br /> | ||
| isButtonDisabled 옵션으로 disabled state를 확인해보세요. | ||
| </> | ||
| </Callout> | ||
| <Callout {...(Information.args as CalloutProps)} buttonLabel='hover, press 해보세요!'> | ||
| <> | ||
| 버튼이 있는 경우 hasIcon과 무관하게 아이콘이 항상 표시돼요. <br /> | ||
| isButtonDisabled 옵션으로 disabled state를 확인해보세요. | ||
| </> | ||
| </Callout> | ||
| <Callout {...(Warning.args as CalloutProps)} buttonLabel='hover, press 해보세요!'> | ||
| <> | ||
| 버튼이 있는 경우 hasIcon과 무관하게 아이콘이 항상 표시돼요. <br /> | ||
| isButtonDisabled 옵션으로 disabled state를 확인해보세요. | ||
| </> | ||
| </Callout> | ||
| </div> | ||
| ), | ||
| }; | ||
|
|
||
| // 여러줄 텍스트 콜아웃 스토리 | ||
| export const CalloutWithLongText: StoryObj<CalloutProps> = { | ||
| args: { | ||
| children: ( | ||
| <> | ||
| 버튼이 있는 경우 hasIcon과 무관하게 아이콘이 항상 표시돼요. <br /> | ||
| isButtonDisabled 옵션으로 disabled state를 확인해보세요. | ||
| </> | ||
| ), | ||
| type: "warning", | ||
| children: | ||
| 'Facebook 정책이 변경되어, 앞으로 Facebook 로그인이 불가해요. 다른 계정으로 재설정 부탁드려요. Facebook 정책이 변경되어, 앞으로 Facebook 로그인이 불가해요. 다른 계정으로 재설정 부탁드려요.', | ||
| type: 'information', | ||
| hasIcon: true, | ||
| buttonLabel: "hover, press 해보세요!", | ||
| buttonLabel: '소셜 계정 재설정하기', | ||
| isButtonDisabled: false, | ||
| }, | ||
| }; | ||
|
|
||
| // 여러줄 텍스트 콜아웃 스토리 | ||
| export const CalloutWithLongText: StoryObj<CalloutProps> = { | ||
| export const CalloutWithShortText: StoryObj<CalloutProps> = { | ||
| args: { | ||
| children: | ||
| "Facebook 정책이 변경되어, 앞으로 Facebook 로그인이 불가해요. 다른 계정으로 재설정 부탁드려요. Facebook 정책이 변경되어, 앞으로 Facebook 로그인이 불가해요. 다른 계정으로 재설정 부탁드려요.", | ||
| type: "information", | ||
| children: '짧은 텍스트 ', | ||
| type: 'information', | ||
| hasIcon: true, | ||
| buttonLabel: "소셜 계정 재설정하기", | ||
| buttonLabel: '짧은 라벨', | ||
| isButtonDisabled: false, | ||
| }, | ||
| }; |
|
Contributor
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에서 특별하게 스크린 리더에 담길 내용이 없다면 아이콘 2개 다
Author
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. 098b26e 좋은 리뷰 감사합니다 반영완료했습니다 ~ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import type { CSSProperties } from "react"; | ||
| import theme from "../theme.css"; | ||
| import type { CalloutType } from "./types"; | ||
| import type { CSSProperties } from 'react'; | ||
| import theme from '../theme.css'; | ||
| import type { CalloutType } from './types'; | ||
|
|
||
| export const iconColors: Record<CalloutType, string> = { | ||
| danger: theme.colors.red500, | ||
|
|
@@ -18,7 +18,7 @@ export const calloutColors: Record<CalloutType, CSSProperties> = { | |
| borderColor: theme.colors.blueAlpha600, | ||
| }, | ||
| warning: { | ||
| backgroundColor: "rgba(255, 194, 52, 0.1)", | ||
| backgroundColor: 'rgba(255, 194, 52, 0.1)', | ||
|
Contributor
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. 해당 색상이 하드 코딩 되어있는데 저희 mds에 추가가 안되어있네요.. 추가로 피그마에도 반영이 안되어있어서
Author
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. 넵 이부분 가온님께 요청드렸고 추후 머지되면 한 번 더 확인하겠습니다~ |
||
| borderColor: theme.colors.yellow600, | ||
| }, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| export { default } from "./Callout"; | ||
| export { default } from './Callout'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| export type CalloutType = "danger" | "information" | "warning"; | ||
| export type CalloutType = 'danger' | 'information' | 'warning'; |
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.
변경된 부분은 아니긴한데 아래 Icon의 visible 조건에
buttonLabel가 같이 있는데 이렇게 되면hasIcon이false여도buttonLabel이 있으면 아이콘이 뜨는 case가 생길 것 같아요!그리고
hasIcon이optional인데 초기 값이 없어서 만약buttonLabel을 조건에서 없애게 되면 기본 값 지정을 해주면 어떨까요?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.
buttonLabel을 넣은 것은 해당 문서(이미지)를 보고 'buttonLabel이 존재하는 케이스'에서는 무조건 icon이 존재한다고 판단했기 때문이에요! 그렇게 사용되어야한다면 variant를 활용해 icon, default, buttonLabel로 나누었으면 좋았겠다는 생각이 드네요.
피그마 문서 링크도 남깁니다! 다만
Callout디자인시스템을 플그만 사용하고 있어서 영향이 가지 않는 선에서 더 좋은 개선방안을 고민해보겠습니다!https://www.figma.com/design/ZuAGH1zZDxduHsjCZm8kfH/%F0%9F%9A%80-makers-design-system?node-id=9198-6952&m=dev

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.
aeb722f 초기값 추가했습니다 감사합니다.