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.

Sign up for GitHub

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

Jump to bottom

모임 피드 아이템 보완 #429

Merged
merged 7 commits into from
Sep 23, 2023
Merged

모임 피드 아이템 보완 #429

merged 7 commits into from
Sep 23, 2023

Conversation

100Gyeon
Copy link
Member

🚩 관련 이슈

📋 작업 내용

  • 제목 말줄임 처리 (40자 초과 시)
  • 콘텐츠 말줄임 처리 (70자 초과 시)
  • 추가 이미지 개수 표시
  • 더보기 버튼 주석 처리
  • 댓글 부분에 Overlay 인터페이스 반영

📌 PR Point

  • 글자 수를 기준으로 말줄임 처리가 필요한 경우, utils에 있는 truncateText를 활용해 주세요 😉
  • 프사가 없는 경우를 대비해 Avatar.tsx에 <ProfileDefaultIcon />을 추가했어요.
  • 이번 릴리즈에는 수정/삭제 기능이 포함되지 않아 더보기 버튼을 숨겼어요.

📸 스크린샷

image

@100Gyeon 100Gyeon self-assigned this Sep 17, 2023
@100Gyeon 100Gyeon temporarily deployed to development September 17, 2023 13:43 — with GitHub Actions Inactive
na-reum
na-reum approved these changes Sep 17, 2023
View reviewed changes
src/components/page/meetingDetail/Feed/FeedItem.tsx Outdated
Comment on lines 176 to 178
backgroundColor: colors.black100,
opacity: 0.6,
color: colors.gray30,
Copy link
Contributor

Choose a reason for hiding this comment

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

우리가 원래 썻던 방식대로 써도 colors 다 사용 가능하니까 그렇게 써주세여~

Copy link
Member Author

Choose a reason for hiding this comment

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

역시 꼼꼼해.. 반영했어 !!

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Sep 17, 2023 •
edited
Loading

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2ac0f96
Status: ✅  Deploy successful!
Preview URL: https://378fd30a.sopt-crew-dev-legacy.pages.dev
Branch Preview URL: https://feat--411.sopt-crew-dev-legacy.pages.dev

View logs

@100Gyeon 100Gyeon temporarily deployed to development September 17, 2023 13:47 — with GitHub Actions Inactive
borimong
borimong reviewed Sep 18, 2023
View reviewed changes
stitches.config.ts
@@ -279,7 +279,7 @@ const stitches = createStitches({
170: '170%',
H1: '24px',
H2: '30px',
H3: '18px',
H3: '28px',
Copy link
Contributor

Choose a reason for hiding this comment

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

최고!!!

borimong
borimong reviewed Sep 18, 2023
View reviewed changes
src/utils/truncateText.ts
@@ -0,0 +1,6 @@
export default function truncateText(text: string, limit: number) {
Copy link
Contributor

@borimong borimong Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

지연언니 진짜 너무 머쪄..!!! 🚀🚀 🥰 🥰

borimong
borimong reviewed Sep 18, 2023
View reviewed changes
src/components/page/meetingDetail/Feed/FeedItem.tsx
<Avatar
key={`${thumbnail}-${index}`}
src={thumbnail}
alt=""
Copy link
Contributor

Choose a reason for hiding this comment

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

alt 혹시 깜빡한 거라면..!! 적어주면 더 좋게따!! 😄😄

Copy link
Member Author

Choose a reason for hiding this comment

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

요거는 프로필 사진이라 alt를 일부러 비웠어! (참고)

한 가지 고려할 부분은 이미지 내용에 의미가 포함되어 있는지, 아니면 순전히 시각적인 장식을 위한 것인지 입니다. 그저 장식 요소라면 alt 속성에는 빈 문자열을 작성하거나 CSS background로 페이지에 포함시키는 편이 좋습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

오오 그렇구나..!! 덕분에 배웠어 :) 😍🌷

borimong
borimong reviewed Sep 18, 2023
View reviewed changes
src/components/page/meetingDetail/Feed/FeedItem.tsx Outdated
<SContent>{truncateText(contents, 70)}</SContent>
{images && (
<SThumbnailWrapper>
<SThumbnail src={images[0]} alt="" />
Copy link
Contributor

@borimong borimong Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

alt 혹시 깜빡한 거라면..!! 적어주면 더 좋게따!! 😄😄
그리구 오늘 재훈오빠한테서 스을쩍 들었는데 매직넘버를 지양하면 좋다구 그래서..! 40, 70, 0 같은 상수를 titleMaxLength, contentMaxLength, startIndex 같은 변수에 저장한 뒤에 사용해보면 어떨까아??? (@na-reum 맞나요 아빠,,)

Copy link
Member Author

Choose a reason for hiding this comment

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

alt는 위에서 언급한 거랑 동일한 이유라서 생략할게 ~~

상수는 아래처럼 반영하면 되겠다!
constants/index.ts (공통) -> START_INDEX
constants/feed.ts (피드) -> TITLE_MAX_LENGTH, CONTENT_MAX_LENGTH

Copy link
Contributor

Choose a reason for hiding this comment

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

@borimong 응 맞아!
사실 나도 타이틀 부분에서 매직넘버 사용한 부분이 있는데, 수정해야겠다 ㅎㅎ
@100Gyeon
TITLE_MAX_LENGTH 가 폼에서는 타이틀이 100자 제한이지만, 카드에서는 70자이니까
FORM_TITLE_MAX_LENGTH 랑, CARD_TITLE_MAX_LENGTH 로 구분하는건 어떨까?

Copy link
Contributor

@na-reum na-reum Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

START_INDEX 도 이렇게만 적으면,
왜 start index를 image에 넣었는지 이유를 모르니 매직넘버를 확실하게 해결했다고 하기엔 어려울것 같아!
우리가 image 중에 첫번째를 thumbnail image로 사용하고 있으니까
THUMBNAIL_IMAGE_INDEX로 명확하게 구분하는건 어때?

Copy link
Member Author

Choose a reason for hiding this comment

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

웅웅 좋아 오빠가 말한 부분까지 반영해서 수정할게~!

Copy link
Contributor

Choose a reason for hiding this comment

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

최고최고!!! 🚀🚀

borimong
borimong approved these changes Sep 18, 2023
View reviewed changes
Copy link
Contributor

@borimong borimong left a comment

Choose a reason for hiding this comment

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

지 연 언 니 최 고 최 고 ! ! !

@100Gyeon 100Gyeon temporarily deployed to development September 20, 2023 02:00 — with GitHub Actions Inactive
eunsukimme
eunsukimme approved these changes Sep 23, 2023
View reviewed changes
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.

고생했어~~ 👍

@100Gyeon 100Gyeon merged commit 2b60928 into develop Sep 23, 2023
@100Gyeon 100Gyeon deleted the feat/#411 branch September 23, 2023 09:23
@borimong borimong mentioned this pull request Sep 23, 2023
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@na-reum na-reum na-reum approved these changes

@borimong borimong borimong approved these changes

@eunsukimme eunsukimme eunsukimme approved these changes

Assignees

@100Gyeon 100Gyeon

Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

모임 피드 아이템 보완
4 participants
@100Gyeon @na-reum @eunsukimme @borimong