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/#269 qa #284

Merged
merged 13 commits into from
May 5, 2023
Merged

Feat/#269 qa #284

merged 13 commits into from
May 5, 2023

Conversation

joohaem
Copy link
Member

@joohaem joohaem commented May 4, 2023

  • 브랜치명, 브랜치 알맞게 설정
  • Reviewer, Assignees, Label, Milestone, Issue(PR 작성 후에) 붙이기
  • PR이 승인된 경우 해당 브랜치는 삭제하기

📌 내용

QA 작업 마쳤습니다. 빠뜨린 게 있을 수 있어요..!

  • 북마크 클릭 시 나오는 로그인 팝업 마감 처리 부분
  • 피클미에서 나오는 로그인 팝업창
  • 카드뷰에서 개발 카드 수정 사항들 체크
  • 전체 카드들이 시작하는 지점 수정
  • 카드뷰 헤더 인터렉션 헤더로 수정
  • CTA 버튼 수정
  • 업데이트 알림 텍스트 수정
  • 대화주제 메들리 new 새로운 기능 등 강조된 부분

📌 Comment

  • HeadlessCTAButton 이라는 공통 컴포넌트를 만들었어요. 같은 디자인의 경우 이를 활용하면 좋을 것 같아요!

@joohaem joohaem requested a review from NYeonK May 4, 2023 10:09
@joohaem joohaem self-assigned this May 4, 2023
@joohaem joohaem linked an issue May 4, 2023 that may be closed by this pull request
10 tasks
Copy link
Contributor

@NYeonK NYeonK 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 +12 to +13
setIsScrollTop(currentScrollPosition < 20);
setIsScrollingDown(lastScrollPositionRef.current < currentScrollPosition);
Copy link
Contributor

Choose a reason for hiding this comment

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

이 부분에 값이 어떻게 들어가는건지 궁금합니다!!👀👀

그나저나 isScrollingDown, isScrollTop 변수명 대박인데요? 세심한 사람✨✨

Copy link
Member Author

Choose a reason for hiding this comment

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

스크롤된 정도가 20 이하면, isScrollTop 상태이고
이전 스크롤 정도보다 현재 스크롤 정도가 크다면 isScrollingDown 상태입니다!

하지만 이 훅.. 사용하지 않습니다 ㅋㅋㅋ intersectionObeserver로 첫 카드 넘어갈 때 헤더가 바뀌는 방식으로 바꾸었어요
해당 훅은 이후에 쓰일까 남겨두었습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

아하!!! 이해했어요💡 어쩐지 없드라😎😎

<Card
openLoginModalHandler={openLoginModalHandler}
{...cardList}
firstCardObsvRef={idx === 0 ? firstCardObsvRef : undefined}
Copy link
Contributor

Choose a reason for hiding this comment

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

undefined 대신 null은 어떠신지요?-?

Copy link
Member Author

Choose a reason for hiding this comment

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

null 은 object 타입으로, "값이 비어있음을 나타내는 유효한 값"이라고 생각해요
따라서 아에 아무것도 아님을 표현하기 위해 undefined로 작성해보았어요!

Copy link
Contributor

Choose a reason for hiding this comment

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

아무것도 아님을 표현하는 거였군용!!🤩 저는 값이 없음을 표현하는 줄 알고 제안해봤습니다:)

Comment on lines +8 to +15
// MEMO :: 페이지 방문 시 매번 Open 설정 요구 (기획 측)
// useEffect(() => {
// const isPopupShown = sessionStorage.getItem(POPUP_SESSION_KEY);
// if (!isPopupShown) {
// setIsOpened(true);
// sessionStorage.setItem(POPUP_SESSION_KEY, "true");
// }
// }, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

MEMO 확인이욤~!~!!

Comment on lines 34 to 39
background-color: ${({ theme }) => theme.colors.gray900};
border-radius: 0.8rem;

backdrop-filter: blur(0.6rem);
color: ${({ theme }) => theme.colors.white};
${({ theme }) => theme.fonts.btn2};
Copy link
Contributor

Choose a reason for hiding this comment

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

theme.new~~로 바꾸면 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

와우 감사합니다!!!!


type HeadlessCTAButtonProps = React.ButtonHTMLAttributes<HTMLButtonElement>;

const HeadlessCTAButton = (props: PropsWithChildren<HeadlessCTAButtonProps>) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

변수명에 Headless 이거 왜 들어간 걸까요?! 궁금쓰!!

Copy link
Member Author

Choose a reason for hiding this comment

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

HeadlessUI 에 고안해서 작성했어요! 이는 쉽게 말해 비즈니스 로직(동작)이 전혀 없이 스타일링 로직만 보여줌을 말합니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

HeadlessUI 함 찾아볼게요:) 친절한 설명 감사합니당😆👍

@NYeonK
Copy link
Contributor

NYeonK commented May 4, 2023

로그인 팝업창에서 버튼 사이로 미세하게 안맞는게 보여서요!
아래 코드에서 margin-top: 6.9rem; 이 부분을 6.4rem으로 바꾸면 될 것 같아요!

// LoginModal/style.ts
export const Container = styled.section`
  display: flex;
  flex-direction: column;
  justify-content: center;
  align-items: center;
  gap: 6rem;
  margin-top: 6.9rem;
  height: 13.6rem;
`;

image

@joohaem joohaem requested a review from NYeonK May 4, 2023 15:23
@NYeonK
Copy link
Contributor

NYeonK commented May 4, 2023

올라리 오빠 PR에 바로 올라가넹😅😏 줄바꿈 관련 style 수정했씁니다~!

@joohaem joohaem merged commit baad5c9 into release/1.4.0 May 5, 2023
@joohaem joohaem deleted the feat/#269-qa branch May 5, 2023 00:33
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.

[ Common ] QA 반영
2 participants