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 추가 #430

Closed
wants to merge 1 commit into from
Closed

포스트 헤더 ui 추가 #430

wants to merge 1 commit into from

Conversation

borimong
Copy link
Contributor

@borimong borimong commented Sep 17, 2023

🚩 관련 이슈

📋 작업 내용

  • 포스트 헤더 ui 추가

📌 PR Point

  • 무슨 이유로 어떻게 코드를 변경했는지
  • 포스트 페이지 (피드 상세 페이지)에 헤더 ui 를 추가했어요!
  • 어떤 위험이나 우려가 발견되었는지
  • 어떤 부분에 리뷰어가 집중해야 하는지
  • 개발하면서 어떤 점이 궁금했는지

📸 스크린샷

image
image

@borimong borimong temporarily deployed to development September 17, 2023 16:18 — with GitHub Actions Inactive
@cloudflare-workers-and-pages
Copy link

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 229ea46
Status: ✅  Deploy successful!
Preview URL: https://66d51c3e.sopt-crew-dev-legacy.pages.dev
Branch Preview URL: https://feat--414.sopt-crew-dev-legacy.pages.dev

View logs

@borimong borimong changed the title feat: 포스트 헤더 ui 추가 포스트 헤더 ui 추가 Sep 17, 2023
@borimong borimong marked this pull request as ready for review September 21, 2023 12:35
Copy link
Contributor

@na-reum na-reum left a comment

Choose a reason for hiding this comment

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

고생하셨어요

Comment on lines +46 to +48
<SAvatar src={post.user.profileImage || ''} alt={post.user.name} />
<AuthorInfo>
<AuthorName>{post.user.name}</AuthorName>
Copy link
Contributor

Choose a reason for hiding this comment

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

스크린리더 사용자가 AuthorName으로 name의 정보를 얻을 수 있으니 Avatar는 시각적인 효과만 낼수있도록 alt를 ""로 작성하는것이 좋을것 같아요

Copy link
Member

Choose a reason for hiding this comment

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

alt 내가 넣어둔건데 그렇게 하는게 좋겠네요~

{post.images && (
<ImageSection>
{post.images.length === 1 ? (
<BigImage src={post.images[0]} onClick={handleClickImage(post.images, 0)} />
Copy link
Contributor

Choose a reason for hiding this comment

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

여기도 나중에 지연이 pr 머지되면 매직넘버 없애보자

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
Member

Choose a reason for hiding this comment

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

@na-reum 머지했습니다 ~~!

Copy link
Contributor Author

@borimong borimong Sep 23, 2023

Choose a reason for hiding this comment

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

@eunsukimme
#429
요기에서 '매직넘버' 검색하면 우리가 했던 열띤 토론을 볼 수 있습니댯,,,,ㅎㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

여기도 나중에 지연이 pr 머지되면 매직넘버 없애보자

좋아!!!!!!!!

<Contents>{post.contents}</Contents>
{post.images && (
<ImageSection>
{post.images.length === 1 ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

hasOnlyThumbnail 과 같은 말로 바꿀 수 있을것 같아

Copy link
Member

Choose a reason for hiding this comment

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

변수로 인라인하는 건 좋은 것 같당! 그런데 hasOnlyThumbnail 란 이름이 '이미지가 하나만 있다'랑 완전 같은 의미는 아닌 것 같은데 좀 더 찰떡인 이름이 없을까.. 🤔 이름 짓기 넘 어렵군 ㅜ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hasOnlyOnePostImage 는 어때요오,,,?? ㅎㅎ

Comment on lines +86 to +97
<CommentLikeWrapper>
<CommentLike>
<CommentIcon />
{/* TODO: add comment count */}
<span style={{ marginLeft: '4px' }}>댓글 </span>
</CommentLike>
<Divider />
<CommentLike>
{post.isLiked ? <LikeFillIcon /> : <LikeIcon />}
<span style={{ marginLeft: '4px' }}>좋아요 {post.likeCount}</span>
</CommentLike>
</CommentLikeWrapper>
Copy link
Contributor

Choose a reason for hiding this comment

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

프리티어 설정이 다른가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

앗 요오거는 아마 Header 부분 추가하느라 전체를 <></>로 한 번 더 감싸주어서 이렇게 된 거 같아요..!! 근데 이 부분뿐만 아니라 전체적으로 다... 그래서 은수오빠가 한 부분들까지 다 + 되어버리는 현상이 일어난...것 같숨니당......ㅠ^ㅠ

Copy link
Contributor

Choose a reason for hiding this comment

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

아하 이해했어염
그냥 뎁스가 깊어진거군 발견 못했네!

Copy link
Member

@eunsukimme eunsukimme left a comment

Choose a reason for hiding this comment

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

고생했어요 👍

@borimong borimong closed this Jan 29, 2024
@100Gyeon 100Gyeon deleted the feat/#414 branch February 5, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

피드 상세 헤더 UI 추가
4 participants