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

Feat/toast message 최종 개선 #650 #668

Closed

Conversation

Wilbur0306
Copy link
Contributor

@Wilbur0306 Wilbur0306 commented Feb 20, 2023

📌 개요

  • toast message 최종 수정

💻 작업사항

  • 저번 PR 리뷰에서 받은 커멘트들 기반으로 toast message 최종 수정
  • 관리자 전체 유저 알림 모달과 toast message 기능 연결

시연

  1. npm i & npm run dev 실행한다
  2. 브라우저에서 "localhost:3000/admin" 접속 후 관리자 페이지의 "알림 관리" 들어간다
  3. 우측 상단에 있는 "All" 버튼을 누른다
  4. [적용] 버튼을 누른 뒤 좌측 하단에 뜨는 Success 토스트 메세지 확인

관련 이슈

@Wilbur0306 Wilbur0306 requested a review from a team as a code owner February 20, 2023 06:30
@Wilbur0306 Wilbur0306 requested review from mike2ox, raehy19 and 42sungwook and removed request for a team February 20, 2023 06:30
@Wilbur0306 Wilbur0306 self-assigned this Feb 20, 2023
@Wilbur0306 Wilbur0306 added the admin 관리자 페이지에서 발생한 issue label Feb 20, 2023
@Wilbur0306 Wilbur0306 requested a review from a team February 20, 2023 06:31
Copy link
Contributor

@42sungwook 42sungwook left a comment

Choose a reason for hiding this comment

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

고생하셨습니다. 몇가지만 수정해주시면 될 것 같아요

components/modal/admin/AdminNotiAllModal.tsx Outdated Show resolved Hide resolved
components/modal/admin/AdminNotiAllModal.tsx Show resolved Hide resolved
useEffect(() => {
console.log({ allNoti });
return () => {
console.log({ allNoti });
};
}, [allNoti]);
const setModal = useSetRecoilState(modalState);
const finishEditHandler = () => setModal({ modalName: null });
Copy link
Contributor

Choose a reason for hiding this comment

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

이 핸들러가 현재 안쓰이는데 handleclick에 넣으시면 될 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[적용] 버튼을 눌렀을 때에 Success 토스트 메세지가 띄워짐과 동시에 모달 창만 닫히는 것으로 구현을 해야할지, 아니면 모달 창은 그대로 있는 상태에서 토스트 메세지만 띄워져야할지 상의 후 결정 나는 로직대로 수정하겠습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

이거는 알림 보낸게 table에 그대로 남으니까 모달창 꺼줘도 될꺼 같아요.

Copy link
Contributor

Choose a reason for hiding this comment

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

그리고 finishEditHandler와 cancelEditHandler 둘다 동작 로직이 동일해서 개선이 필요해 보여요
finishEditHandler같은 경우 동작 시 toast가 어떤 형식으로든 나타날 테니 finishEditHandler 부븐을 수정하면 어떨까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

모송님 의견에 동의합니다 짧은 내용이라 handler를 굳이 함수로 선언하기보다는 사용하는곳에 화살표함수로 setmodal null을 실행하는것도 좋을것같다고 생각해용

components/toastmsg/toastmsg.tsx Outdated Show resolved Hide resolved
@42sungwook
Copy link
Contributor

지금 모달창이 꺼지면 토스트 메시지도 사라지는 종속관계인데 recoil을 사용해서 독립적으로 메시지를 띄우게 바꿔보시면 좋을 것 같습니다!

Copy link
Contributor

@mike2ox mike2ox left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 빠르게 건의했던거 확인하시고 내일 회의전에 수정후 컨펌 받으시고 merge gogo!!!

components/modal/admin/AdminNotiAllModal.tsx Outdated Show resolved Hide resolved
useEffect(() => {
console.log({ allNoti });
return () => {
console.log({ allNoti });
};
}, [allNoti]);
const setModal = useSetRecoilState(modalState);
const finishEditHandler = () => setModal({ modalName: null });
Copy link
Contributor

Choose a reason for hiding this comment

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

그리고 finishEditHandler와 cancelEditHandler 둘다 동작 로직이 동일해서 개선이 필요해 보여요
finishEditHandler같은 경우 동작 시 toast가 어떤 형식으로든 나타날 테니 finishEditHandler 부븐을 수정하면 어떨까요?

components/modal/admin/AdminNotiAllModal.tsx Outdated Show resolved Hide resolved

return (
<div className={styles.whole}>
<div className={styles.body}>
<div className={styles.title}>NOTI FOR ALL</div>

<label className={styles.body}>
To: everyone
<input
<textarea
Copy link
Contributor

Choose a reason for hiding this comment

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

따로 입력길이 제한은 안넣는걸까요?
기존 notification 데이터 저장할 때 제한 조건이 없었다면 사용자측에 보여지는 방식을 한번 봐야할 거 같네요. (ex. 입력 데이터가 너무 길 경우)

components/modal/admin/AdminNotiAllModal.tsx Outdated Show resolved Hide resolved
components/toastmsg/toastmsg.tsx Outdated Show resolved Hide resolved
Comment on lines +12 to +17
const Alert = forwardRef<HTMLDivElement, AlertProps>(function Alert(
props,
ref
) {
return <MuiAlert elevation={6} ref={ref} variant='filled' {...props} />;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

몇가지 궁금한게 있어서 설명해주시면 좋을거 같습니다!

  • forwardRef의 기능
  • MuiAlert에서 elevation의 역할

components/toastmsg/toastmsg.tsx Outdated Show resolved Hide resolved

return (
<Stack spacing={2} sx={{ width: '100%' }}>
<Snackbar open={open} autoHideDuration={6000} onClose={handleClose}>
Copy link
Contributor

Choose a reason for hiding this comment

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

autoHideDuration에 들어가는 인자도 상수화 해주는게 좋을거 같네요.
현재 main에 constants 폴더 안에 구성해 놓은게 있으니 merge하셔서 확인해보세요

styles/admin/modal/AdminNotiAll.module.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@raehy19 raehy19 left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니당 소소한 코멘트 확인해주세요 !

return <MuiAlert elevation={6} ref={ref} variant='filled' {...props} />;
});

export default function CustomizedSnackbars({
Copy link
Contributor

Choose a reason for hiding this comment

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

파일이름과 컴포넌트 이름은 같은게 보기 좋을것같아요 !

components/toastmsg/toastmsg.tsx Outdated Show resolved Hide resolved
components/toastmsg/toastmsg.tsx Outdated Show resolved Hide resolved
styles/admin/modal/AdminNotiAll.module.scss Outdated Show resolved Hide resolved
styles/admin/modal/AdminNotiAll.module.scss Outdated Show resolved Hide resolved
components/modal/admin/AdminNotiAllModal.tsx Show resolved Hide resolved
useEffect(() => {
console.log({ allNoti });
return () => {
console.log({ allNoti });
};
}, [allNoti]);
const setModal = useSetRecoilState(modalState);
const finishEditHandler = () => setModal({ modalName: null });
Copy link
Contributor

Choose a reason for hiding this comment

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

모송님 의견에 동의합니다 짧은 내용이라 handler를 굳이 함수로 선언하기보다는 사용하는곳에 화살표함수로 setmodal null을 실행하는것도 좋을것같다고 생각해용

@Wilbur0306
Copy link
Contributor Author

Wilbur0306 commented Feb 22, 2023

지금 모달창이 꺼지면 토스트 메시지도 사라지는 종속관계인데 recoil을 사용해서 독립적으로 메시지를 띄우게 바꿔보시면 좋을 것 같습니다!
수정하였습니다!

@Wilbur0306 Wilbur0306 closed this Feb 22, 2023
@Wilbur0306 Wilbur0306 reopened this Feb 22, 2023
@Wilbur0306 Wilbur0306 closed this Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admin 관리자 페이지에서 발생한 issue
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Feat] toast message 기능 추가 [Feat] 관리자 전체 유저 알림 모달 개선
4 participants