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

[#227, SEGAM-33] 공지사항 긴급 공지 팝업 로직 #273

Open
wants to merge 3 commits into
base: add/notice-popup-224
Choose a base branch
from

Conversation

tnqkr3494
Copy link
Member

@tnqkr3494 tnqkr3494 commented Jan 19, 2025

작업사항

resolves #227

localStorage, sessionStorage를 이용해서 모달 팝업 기능구현 완료하였습니다!

@tnqkr3494 tnqkr3494 self-assigned this Jan 19, 2025
Copy link

vercel bot commented Jan 19, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
segam-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 19, 2025 7:10am
segam-web-develop ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 19, 2025 7:10am

@tnqkr3494 tnqkr3494 changed the title [#227] feat: storage 이용한 팝업 기능 구현 [#227, SEGAM-33] feat: storage 이용한 팝업 기능 구현 Jan 19, 2025
@tnqkr3494 tnqkr3494 changed the title [#227, SEGAM-33] feat: storage 이용한 팝업 기능 구현 [#227, SEGAM-33] 공지사항 긴급 공지 팝업 로직 Jan 19, 2025
Copy link
Collaborator

@kmsu44 kmsu44 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

sessionStorage.removeItem('seen');
}
const noticeModal = () => {
Modal.confirm({
Copy link
Collaborator

Choose a reason for hiding this comment

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

confirm모달 기존에 useModal의 confirm모달을 사용하면 좋을 것 같습니다.

디자인 요구사항이 달라 커스텀이 필요한 경우는 추가적으로 props를 확장하는 방향으로 가는게 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

confirm 모달 재사용해봤습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

작성해 주신 방법 처럼 isNotice을 사용해서 특정상황에 맞춰서 커스텀 하는 방식도 좋습니다!!

추가적으로 다음과 같은 방법도 있으니 참고 하시면 좋을 것 같습니다!

import { Modal, ModalProps } from 'antd';

interface SegamModalProps extends ModalProps {
 ~~~
}

기본적으로 저희가 antd 라이브러리의 컴포넌트를 사용하고 있기 때문에 해당 props 타입을 extend로 확장하게 되면 해당 타입을 따로 정의하지 않고 가져올 수 있는데요!

다음 방식을 사용하면 confirmModal에 대해서 notice라는 의존성을 분리할 수 있어 재사용성이 늘어나는 효과를 볼 수 있습니다:)

각 방식에 장단점에 대해서는 한번 고민해보시면 좋을 것 같아요!!

src/lib/actions/noticePopUp.ts Outdated Show resolved Hide resolved
Copy link
Member

@luciancah luciancah left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 리뷰중에 수정사항이 올라와서 리졸브 될만한 사항이 있을수도 있겠네요.
제가 드리는 리뷰는 어디까지나 제 의견이라서 참고만 하시면 되겠습니다.


export default function NoticeModal({ noticeData }: { noticeData: Notice }) {
const { noticeModal } = useModal();
const { title, content, id } = noticeData;

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

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

useEffect 부분을 분리하는 것도 좋은 아이디어 같습니다..
dismissCheck !== id, seenCheck !== id, !seenCheck && !dismissCheck 3가지로 useEffect 부 내부의 로직이 나뉘는 것 같은데요, noticeModal 선언부가 useEffect 안에 들어가 있어서 로직 따라가기가 힘들어 보여요

Copy link
Member

Choose a reason for hiding this comment

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

저였으면 외부에

const showNoticeModal = useCallback(() => {
    Modal.confirm({
      title: <h3 className="f20 mb-4 font-bold text-text_primary">{title}</h3>,
      content: <p className="f16 font-medium text-text_primary">{content}</p>,
      footer:  ....

선언하고 useEffect 부분 로직을 깔끔하게 가져갈 것 같습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SEGAM-33: [FE] 공지사항 공지사항 긴급 공지 팝업 로직
3 participants