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

공지사항 목록 컴포넌트 / 공지사항 상세 컴포넌트 UI 구현 / 공지사항 ROUTER 설정 #749

Merged
merged 21 commits into from
Oct 18, 2023

Conversation

Gilpop8663
Copy link
Collaborator

🔥 연관 이슈

close: #741

📝 작업 요약

  • 공지사항 목록 컴포넌트 구현 및 페이지 구현
  • 공지사항 상세 컴포넌트 구현 및 페이지 구현
  • 공지사항 router 설정

⏰ 소요 시간

3시간

🔎 작업 상세 설명

  • 공지사항 상세 페이지를 구현
-.VoTogether.-.Brave.2023-10-16.00-12-48.mp4
-.VoTogether.-.Brave.2023-10-16.00-14-01.mp4
  • 공지사항 목록 페이지를 구현
-.VoTogether.-.Brave.2023-10-16.00-25-29.mp4

image

  • 대쉬보드에 공지사항 보러가기 추가

image

  • 공지사항 router 설정

/notices : 공지사항 목록 페이지
/notices/:id 공지사항 상세 페이지

@github-actions
Copy link

github-actions bot commented Oct 15, 2023

⚡️ Lighthouse report!

Category Score
🟠 Performance 61
🟠 Accessibilty 89
🟢 SEO 100
🟠 PWA 89
Category Score
🟢 First Contentful Paint 0.8 s
🟠 Largest Contentful Paint 3.6 s
🔴 Total Blocking Time 2,360 ms
🟢 Cumulative Layout Shift 0
🟢 Speed Index 3.1 s

Copy link
Member

@inyeong-kang inyeong-kang left a comment

Choose a reason for hiding this comment

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

상수화랑 시맨틱 태그 관련 코멘트 남겼습니다😄

/**
* yyyy-mm-dd 형식
*/
createdAt: string;
Copy link
Member

Choose a reason for hiding this comment

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

StringDate 타입 재사용 어떠신가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

제안해주신대로 적용해보았습니다

theme="blank"
onClick={() => {
navigate('/notices');
}}
Copy link
Member

Choose a reason for hiding this comment

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

PATH.NOTICE 어떠신가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

좋은 제안 감사합니다

@@ -0,0 +1,25 @@
import { styled } from 'styled-components';

export const Container = styled.div`
Copy link
Member

Choose a reason for hiding this comment

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

div 대신 main 이나 section 어떠신가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

제안해주셔서 감사해요 main으로 했습니다~

<>
<NoticeDetailPage />
<RouteChangeTracker />
</>
Copy link
Member

Choose a reason for hiding this comment

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

오 공지사항 목록이랑 상세도 RouteChangeTracker 를 추가해주셨군요👍👍

@inyeong-kang
Copy link
Member

fe-리뷰완

Copy link
Collaborator

@chsua chsua left a comment

Choose a reason for hiding this comment

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

어떻게 이렇게 빠르게 만드시는 거죠???
대단해요 우스
페이지가 여러 개이고 새로운 컴포넌트도 여러 개 인데 빠르게 만드는 능력 부럽습니다!
몇 가지 제안 및 코멘트 확인해주세요!!
고생하셨습니다!!!!

interface NoticeDetailProps {
title: string;
content: string;
createdAt: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 그 StringDate로 하지 않으신 이유가 있으신가요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Notice 타입을 다른 PR에서 해서 타입 적용하는데 어려움이 있었어요 ㅠㅠ

제안해주신대로 StringDate로 변경했습니다

export const Title = styled.h1`
margin-top: 20px;

font: var(--text-title);
Copy link
Collaborator

Choose a reason for hiding this comment

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

이거 조금 작은거 같은데 제가 알림 PR에다가 이거보다 큰 --text-page-title인가,, 만들었는데 나중에 그걸로 바꿔도 좋을 것 같아요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

좋아요!

white-space: pre-wrap;
`;

export const ButtonContainer = styled.div`
Copy link
Collaborator

Choose a reason for hiding this comment

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

데스크탑에서는 하단에 홈으로 가기 버튼이 없어도 괜찮을 것 같아요!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아래와 같이 수정해보았습니다~

image

image

/**
* yyyy-mm-dd 형식
*/
createdAt: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

DateString처럼 이것도 타입 강화하는건 어떻게 생각하시나요?

createdAt: string;
}
export default function NoticeItem({ id, title, createdAt }: NoticeItemProps) {
const createdDate = createdAt.slice(0, 10);
Copy link
Collaborator

Choose a reason for hiding this comment

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

들어오는 내용이 yyyy-mm-dd 형식인데 방어코드로 한번 더 잘라주는 건가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

시간은 안보여주는 디자인으로 구현해서 잘라서 보여줬어요

yyyy-mm-dd 형식으로욤

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

헷갈리는 주석을 때문에 혼동이 와서 주석을 삭제했어요 🔥


font: var(--text-body);

transition: color 0.2s ease-in-out;
Copy link
Collaborator

Choose a reason for hiding this comment

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

옹 hover를 위한 애니메이션인가요???

  &:hover {
    color: rgba(51, 122, 183, 1);
  }

여기 안에 넣지 않으신 이유가 있나요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hover안에 transition을 두면 마우스를 치웠을 때는 transition 속성이 작동하지 않아서 hover 밖에 위치시켜주었어요

/**
* yyyy-mm-dd 형식
*/
createdAt: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

모든 공지 컴포넌트에서 yyyy-mm-dd 형식으로 사용된다면 api에서 처음에 데이터를 변환해주시는건 어떻게 생각하세요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

제안해주신대로 api에서 데이터를 변환시켜 주었습니다 👍👍👍

구현하는 과정에서 StringDateOnly 타입이 추가되었어요

/**
 * yyyy-mm-dd HH-MM
 */
export type StringDate = `${number}-${number}-${number} ${number}:${number}`;

/**
 * yyyy-mm-dd
 */
export type StringDateOnly = `${number}-${number}-${number}`;

<Layout isSidebarVisible isChannelTalkVisible={false}>
<S.Container>
<S.Title tabIndex={0}>보투게더 소식</S.Title>
<NoticeList noticeList={MOCK_NOTICE_LIST} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

페이지에서 공지사항리스트를 주입시키신다면 서스펜스와 에러바운더리 처리가 어려울 것 같은데,
혹시 api요청은 페이지에서 하실 예정이신가요?? 아님 공지 목록에서 하실 예정이신가요??🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

공지 목록을 감싸는 NoticeListFetcher를 만들려고 했었습니다

</>
),
},
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

{
  path: PATH.NOTICES,
  element: (
    <>
      <NoticeListPage />
      <RouteChangeTracker />
    </>
  ),
  errorElement: <ErrorPage />,
  children: [
    {
      path: ':noticeId',
      element: (
        <>
          <NoticeDetailPage />
          <RouteChangeTracker />
        </>
      ),
    },
  ],
};

이런 코드는 어떠세요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이렇게 해보았는데요

notices/1 로 url이 변했을 때 :noticeId로 인식을 못하네요

/notices가 먼저 동작하나봐요

image

그래서 원래의 코드로 다시 돌려놓았습니다

@chsua
Copy link
Collaborator

chsua commented Oct 17, 2023

fe-리뷰완

@Gilpop8663
Copy link
Collaborator Author

두 분이 제안해주신 내용을 코드로 반영해보았습니다

구현 과정에서 StringDateOnly 타입이 새로 생겼어요 🔥

fe-리뷰요청

같은 파일에 있어야 코드 가독성이 높아질 것으로 생각했습니다
Copy link
Member

@inyeong-kang inyeong-kang left a comment

Choose a reason for hiding this comment

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

피드백 반영된 코드 확인했습니다~ 타입명 관련 코멘트 하나 남겼어요!

fe-리뷰완

/**
* yyyy-mm-dd
*/
export type StringDateOnly = `${number}-${number}-${number}`;
Copy link
Member

Choose a reason for hiding this comment

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

day까지만 포함시키겠다는 의미로
StringDateUpToDay 어떠신가요? (gpt의 도움을 받았습니다)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

더 명확하게 표현되는 것 같아 반영했습니다~

@Gilpop8663 Gilpop8663 merged commit 8b1872e into dev Oct 18, 2023
1 check passed
@inyeong-kang inyeong-kang deleted the feat/#741 branch October 18, 2023 04:43
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.

공지사항 목록 컴포넌트 / 공지사항 상세 컴포넌트 UI 구현
3 participants