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.

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

[Feature/BAR-158] Dialog 컴포넌트 개발 #39

Merged
merged 9 commits into from
Jan 18, 2024
Merged

[Feature/BAR-158] Dialog 컴포넌트 개발 #39

merged 9 commits into from
Jan 18, 2024

Conversation

miro-ring
Copy link
Contributor

@miro-ring miro-ring commented Jan 15, 2024 •
edited
Loading

Summary

구현 내용 및 작업한 내역을 요약해서 적어주세요

How To Test

Dialog 컴포넌트 정상 노출 확인

@miro-ring miro-ring marked this pull request as draft January 15, 2024 14:38
@miro-ring miro-ring marked this pull request as ready for review January 16, 2024 14:48
wonjin-dev
wonjin-dev approved these changes Jan 17, 2024
View reviewed changes
Copy link
Member

@wonjin-dev wonjin-dev 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/Icon/index.tsx Outdated
@@ -2,15 +2,29 @@ import { iconFactory, type Icons } from '../../constants/icon';

interface IconProps {
icon: Icons;
className?: string;
Copy link
Member

Choose a reason for hiding this comment

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

요게 필요할까요 ?

Copy link
Member

Choose a reason for hiding this comment

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

저도 동의해요! 아이콘(SVG)에 직접 스타일링 하는 대신 태그를 감싸서 스타일링 하는 건 어떤가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wonjin-dev @dmswl98
해당 부분 div 태그로 감싸서 스타일링하는 방식으로 수정하였습니다!

dmswl98
dmswl98 reviewed Jan 17, 2024
View reviewed changes
Copy link
Member

@dmswl98 dmswl98 left a comment

Choose a reason for hiding this comment

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

수고하셨습니당!!
근데 이거 Dialog가 아니라 Dropdown 컴포넌트 아닌가요? 🤔

src/components/Dialog/index.tsx Outdated
Comment on lines 8 to 9
interface DialogContextProps {
type?: 'small' | 'medium';
}
Copy link
Member

@dmswl98 dmswl98 Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

src/components/Dialog/index.tsx
image

src/components/Dialog/components/DialogButton.tsx
image

type에 undefined도 할당 가능한 것으로 보이는데,
type이 옵셔널 타입이라면 초기화되어 있어야 하지 않나용?

현재 상태로는 스타일이 깨질 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmswl98
그렇겟네요!!! 필수 타입으로 변경하였습니다!
(recipe에서 제공하는 헬퍼 fucntion을 사용하면서 undefined 제거하기가 쉽지 않네요...)

src/components/Dialog/index.tsx Outdated
Comment on lines 12 to 13
type DialogRootProps = RecipeVariants<typeof styles.dialogRoot> &
PropsWithChildren<unknown>;
Copy link
Member

Choose a reason for hiding this comment

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

unknown을 사용하신 이유가 궁금해요! 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmswl98
아 요거 default가 unknown 이었네요 ㅎㅎ... 빼서 반영하겠습니다!

src/components/Icon/index.tsx Outdated
@@ -2,15 +2,29 @@ import { iconFactory, type Icons } from '../../constants/icon';

interface IconProps {
icon: Icons;
className?: string;
Copy link
Member

Choose a reason for hiding this comment

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

저도 동의해요! 아이콘(SVG)에 직접 스타일링 하는 대신 태그를 감싸서 스타일링 하는 건 어떤가요?

src/constants/icon.ts Outdated
Comment on lines 9 to 22
export const iconFactory = {
add: Add,
close: Close,
logout: Logout,
profile: Profle,
profileDialog: ProfileDialog,
setting: Setting,
submit: Submit,
Copy link
Member

Choose a reason for hiding this comment

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

알파벳순 정렬 굿 👍🏻👍🏻👍🏻

dmswl98
dmswl98 approved these changes Jan 18, 2024
View reviewed changes
Copy link
Member

@dmswl98 dmswl98 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/Dialog/index.tsx Outdated
Comment on lines 7 to 17
interface DialogContextProps {
type: 'small' | 'medium';
}

interface DialogRootProps extends PropsWithChildren {
type: 'small' | 'medium';
}

export const DialogContext = createContext<DialogContextProps | null>(null);

const DialogRoot = ({ children, type }: DialogRootProps) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
interface DialogContextProps {
type: 'small' | 'medium';
}
interface DialogRootProps extends PropsWithChildren {
type: 'small' | 'medium';
}
export const DialogContext = createContext<DialogContextProps | null>(null);
const DialogRoot = ({ children, type }: DialogRootProps) => {
interface DialogContextProps {
type: 'small' | 'medium';
}
interface DialogRootProps extends DialogContextProps {}
export const DialogContext = createContext<DialogContextProps | null>(null);
const DialogRoot = ({ children, type }: PropsWithChildren<DialogRootProps>) => {

이렇게 작성해볼 수 있을 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3e3ec86
감사합니다! 여기에서 반영해보았습니다 ㅎㅎ

@miro-ring miro-ring merged commit 0c36dad into main Jan 18, 2024
@miro-ring miro-ring deleted the feature/BAR-158 branch January 18, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wonjin-dev wonjin-dev wonjin-dev approved these changes

@dmswl98 dmswl98 dmswl98 approved these changes

Assignees

@miro-ring miro-ring

Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@miro-ring @dmswl98 @wonjin-dev