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

Toast 추가 #83

Merged
merged 5 commits into from
Nov 8, 2023
Merged

Toast 추가 #83

merged 5 commits into from
Nov 8, 2023

Conversation

hae-on
Copy link
Collaborator

@hae-on hae-on commented Nov 4, 2023

Issue

✨ 구현한 기능

toast를 옮겼습니다.

📢 논의하고 싶은 내용

여기의 props는 사실상 사용자가 몰라도 되는 값이라고 생각합니다.
실질적으로 toast.success 이렇게 사용하니까요!
이런 경우는 JSDocs와 리드미 작성을 어떻게 할까요?
그냥 이전 경우와 똑같이 설명을 작성할까요 아니면 props 부분을 적지 말까요??

🎸 기타

x

Copy link
Member

@xodms0309 xodms0309 left a comment

Choose a reason for hiding this comment

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

저도 해온 의견처럼 굳이 사용자한테 props를 전달해줄 필요는 없고 toast.success, toast.error을 어떻게 작성하는지 문서에 남기면 될 것 같아요

근데 디자인시스템 옮기기 전에는

 const { toast } = useToastActionContext();

이런식으로 사용했는데 그럼 index.ts에서 Toast컴포넌트를 export하지 않고 useToastActionContext 요 hook을 export 해야되는 건가요?_?

Copy link
Collaborator

@Leejin-Yang Leejin-Yang left a comment

Choose a reason for hiding this comment

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

해온 코멘트 확인해주세요!

추가적으로 토스트 관련 훅이나 컨텍스트는 Toast 폴더에서 관리하는건 어떤가요??
그리고 사용처에서 사용할 수 있게 export 필요해 보여요.

저도 props 설명은 없어도 된다고 생각합니다. 설명과 예시정도만 있으면 좋겠네요 😊

Comment on lines 1 to 14
import { useContext } from 'react';

import { ToastActionContext } from '../../contexts/ToastContext';

const useToastActionContext = () => {
const toastAction = useContext(ToastActionContext);
if (toastAction === null || toastAction === undefined) {
throw new Error('useToastActionContext는 Toast Provider 안에서 사용해야 합니다.');
}

return toastAction;
};

export default useToastActionContext;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ValueContext인데 ActionContext로 되어 있네요!

Copy link

@wugawuga wugawuga left a comment

Choose a reason for hiding this comment

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

고생했어요 👍

@hae-on
Copy link
Collaborator Author

hae-on commented Nov 5, 2023

@xodms0309 아마 제 예상대로라면 hook을 export 해서 원래 사용하던대로 쓸 거 같아요!
@Leejin-Yang 파일 옮겨봤는데 이렇게 맞나요??? Toast 내부에 context라는 폴더 만들어서 그 안에 때려 넣었습니다. 그리고 export 하라는게 ToastContext를 export 하라는 말이 맞을까요?!?!

@Leejin-Yang
Copy link
Collaborator

@hae-on

그리고 export 하라는게 ToastContext를 export 하라는 말이 맞을까요?!?!

타미말과 동일한 의견이었습니다! useToastActionContext 😊

@hae-on
Copy link
Collaborator Author

hae-on commented Nov 5, 2023

@Leejin-Yang

useToastActionContext 이거 자체에 export 붙여야하나요?
그냥 index에서 export하는건 그냥 경로 depth 줄이기용밖에 안돼요?!?!

@xodms0309
Copy link
Member

@hae-on index 가 entry point라서 저기에서 export해야 하지 않나여?!??

@hae-on
Copy link
Collaborator Author

hae-on commented Nov 8, 2023

@Leejin-Yang @xodms0309
Bottom Sheet처럼 전체 파일 export 해씁니다!
확인 부탁~~

Copy link
Collaborator

@Leejin-Yang Leejin-Yang left a comment

Choose a reason for hiding this comment

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

확인완 수고했어요 해온 🌞

Copy link
Member

@xodms0309 xodms0309 left a comment

Choose a reason for hiding this comment

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

구웃! 이제 잘 작동하는지 확인하는 일만 남았네요 🥹

@hae-on hae-on merged commit 26482ab into main Nov 8, 2023
1 check passed
@hae-on hae-on deleted the feat/issue-80 branch November 8, 2023 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toast 옮기기
4 participants