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: 메인 페이지에 숭대시보 기사 추가 #311

Merged
merged 14 commits into from
Dec 20, 2024

Conversation

nijuy
Copy link
Collaborator

@nijuy nijuy commented Dec 17, 2024

1️⃣ 어떤 작업을 했나요? (Summary)

기존 코드에 영향을 미치지 않는 변경사항

  • 메인 페이지에 숭대시보로 알아보는 교내 소식 패널이 추가되었습니다

    • 기사 하나 클릭하면 새 탭으로 원문이 열립니다
    • 우측 상단의 전체보기 를 누르면 새 탭으로 숭대시보 메인이 열립니다

    news

버그 픽스

2️⃣ 알아두시면 좋아요!

  • 기사 제목 (title)을 key prop 사용 + 응답에 중복 데이터가 존재해서 (관련 스레드)

  • 콘솔 에러가 뜨는데 무시해도 돼용

    image

3️⃣ 추후 작업

  • 리뷰 반영

4️⃣ 체크리스트 (Checklist)

  • main 브랜치의 최신 코드를 pull 받았나요?

@nijuy nijuy added feat New feature or request home labels Dec 17, 2024
@nijuy nijuy self-assigned this Dec 17, 2024
Copy link
Collaborator

@ssolfa ssolfa left a comment

Choose a reason for hiding this comment

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

수고하셨습니다! 코드 깔끔해욤 👍👍👍

저 궁금한 거 질문 하나만 하고!! 어푸룹 갈기겠습니당

PhotoArticleList에서 기사 사진이 없는 경우도 있나 궁금해져서 찾아봤는데

pm님은 이미지 없을 때 이미지 커버가 있었으면 좋겠다고 하셨는데
사진에 대한 분기 처리는 백엔드에서 이미 해준 상태라고 하는데...

뭐가 맞는 건지 궁금합니다!!
만약에 이미지 없는 경우도 있다면 thumbnail이 없는 케이스도 추가해야 되지 않을까~,,, 싶어서요!!!

src/home/components/SsuNews/ArticleList/ArticleList.tsx Outdated Show resolved Hide resolved
src/home/components/SsuNews/SsuNews.tsx Outdated Show resolved Hide resolved
</StyledComponentInnerContainer>
</StyledComponentContainer>
<Spacing direction={'vertical'} size={62} />
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
Member

Choose a reason for hiding this comment

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

Spacing 컴포넌트 대신 StyledComponentContainer의 padding으로 관리하면 어떨까요?

지금은 괜찮지만 Spacing 컴포넌트가 많아지면 JSX 구조가 복잡해질 것 같네요

@nijuy
Copy link
Collaborator Author

nijuy commented Dec 18, 2024

뭐가 맞는 건지 궁금합니다!!
만약에 이미지 없는 경우도 있다면 thumbnail이 없는 케이스도 추가해야 되지 않을까~,,, 싶어서요!!!

이거 오전에 물어봤는데요! (관련 스레드)
thumbnail 필드 값이 null이 되는 일이 없다고 합니다 (˘⌣˘)

Copy link
Member

@2wndrhs 2wndrhs left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 👍

src/home/apis/getNews.ts Show resolved Hide resolved
src/home/types/news.type.ts Outdated Show resolved Hide resolved
</StyledComponentInnerContainer>
</StyledComponentContainer>
<Spacing direction={'vertical'} size={62} />
Copy link
Member

Choose a reason for hiding this comment

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

Spacing 컴포넌트 대신 StyledComponentContainer의 padding으로 관리하면 어떨까요?

지금은 괜찮지만 Spacing 컴포넌트가 많아지면 JSX 구조가 복잡해질 것 같네요

- 서버에서 형식 통일해서 전달해주기로
- replaceAll을 위해 수정했던 버전 회귀
@ssolfa ssolfa self-requested a review December 18, 2024 14:15
Copy link
Member

@seocylucky seocylucky left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!👍🏻👍🏻 늦게 리뷰해서 죄송해여
미리 어푸룹 해두겠습니다!
코멘트 몇 개 달아놨으니 수정하시고 바로 머지해주세요~

{imgNews.map(({ thumbnail, title, date, pageUrl }) => (
<StyledArticleItem key={title} href={pageUrl} target="_blank" rel="noopener noreferrer">
<StyledImg src={thumbnail} alt={'ssu_news_img'} />
<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로 감싸신 이유가 하나의 박스로 묶기 위함인건가요?
다른 코멘트에서 언급드렸던 StyledArticleTitle을 말줄임표 하기 위해서는 이 div 태그의 width를 지정해줘야할 것 같아요!

StyledArticleTtileBox로 이름 지정해서 다음과 같이 작성하는 건 어떨까요?

export const StyledArticleTtileBox = styled.div`
  display: flex;
  flex-direction: column;
  gap: 0.5rem;
  width: 11.4rem;
`;

Copy link
Collaborator Author

@nijuy nijuy Dec 20, 2024

Choose a reason for hiding this comment

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

두 코멘트 답변 모두 여기에 드립니다!

제안하신 말줄임 관련 코드는 긴 텍스트를 한 줄로 표현하는 방식이라 채택하지 않았고,
텍스트를 최대 n줄로 표현하되 그 이상은 말줄임표를 사용하도록 수정했습니다
이미 ResultListItem에서 사용하던 방식이라 보시면 알 거 같아요!

(이 방식은 StyledArticleTitle에 바로 적용이 가능해서 기존 div를 새로운 스타일 요소로 변경하지 않았습니다)

좌측 기사의 말줄임표는 생각하지 못했던 부분인데 감사감사합니다 ദ്ദി˶ˆ꒳ˆ˵)

짧은 제목 긴 제목
image image

Copy link
Member

Choose a reason for hiding this comment

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

오호ㅎㅎㅎㅎㅎ 이해 완료입니다!!! 굳잡잡👍🏻👍🏻👍🏻👍🏻

src/home/components/SSUNews/SSUNews.tsx Outdated Show resolved Hide resolved
<StyledImg src={thumbnail} alt={'ssu_news_img'} />
<div>
<StyledArticleTitle>{title}</StyledArticleTitle>
<StyledDateSpan>입력 | {date.replaceAll('-', '.')}</StyledDateSpan>
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
Collaborator Author

Choose a reason for hiding this comment

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

기사가 입력된 날짜를 의미합니다~! (관련 스레드)
image

@nijuy nijuy merged commit 2f5fd81 into develop Dec 20, 2024
@nijuy nijuy deleted the feat/#310-add-ssu-news branch December 20, 2024 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request home
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants