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/#474] responsive basic setting & main page 임시 responsive 뷰 적용 #475

Merged
merged 14 commits into from
Nov 17, 2024

Conversation

ljh0608
Copy link
Member

@ljh0608 ljh0608 commented Nov 11, 2024

✨ 해당 이슈 번호 ✨

#474

todo

  • Responsive 값 활용하는 responsive context 생성
  • 해당 값으로 mobile only 뷰와 desktop only뷰를 구현하는 Responsive 컴포넌트 생성
  • main page 반응형뷰 임시 적용 (디자이너와 논의 필요)

📌 내가 알게 된 부분

  • Responsive 컴포넌트의 children 컴포넌트에서 width: 100% 같은 값을 사용하면 children 컴포넌트의 부모컴포넌트는 Responsive이기때문에 의도한 대로 동작하지 않는다 그럴 때 asChild props를 통해 Slot을 활용하면 아래와 같이 원래 컴포넌트의 직계 자식 컴포넌트 처럼 활용하여 props뿐만 아니라 style속성도 inherit 받을 수 있다.
  • 슬롯 활용 전후 html 태그 차이 확인
    Slot 활용 전
image Slot 활용 후 image

📌 질문할 부분

  • 없습니다!

📌스크린샷(선택)

image image image

@ljh0608 ljh0608 added the ♻️ Refactor 코드 리팩토링 label Nov 11, 2024
@ljh0608 ljh0608 self-assigned this Nov 11, 2024
Copy link

vercel bot commented Nov 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mile-client ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 12, 2024 5:14pm

@github-actions github-actions bot added the size/l size/l label Nov 11, 2024
Copy link
Member

@namdaeun namdaeun left a comment

Choose a reason for hiding this comment

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

모바일 반응형도 뚝딱뚝딱 최고입니다 ! 고생하셨어요 🚀

type AvailableSize = 'mobile' | 'desktop';

const Responsive = ({ children, only, asChild }: ResponsivePropTypes) => {
const [current, setCurrent] = useState<AvailableSize | null>(null);
Copy link
Member

Choose a reason for hiding this comment

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

P5) current의 초기 값을 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.

current는 추후 useEffect에 브라우저 API를 통해 값이 설정되므로 값이 할당되기 전에는 mobile, desktop이 아닌 null값을 가지고 있어야한다는 판단이였습니다.

Comment on lines +41 to +44
return current === null || only === current ? (
<Comp className={`${selectedClassName()}`}>{children}</Comp>
) : (
<></>
Copy link
Member

Choose a reason for hiding this comment

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

P3) 삼항연산자 대신 && 연산자를 사용해서 불필요한 코드를 최소화 할 수 있을 것 같습니다 ! 아니면 빈 태그를 리턴해주시는 이유가 있을까요 ?

return current === null || only === current && (
    <Comp className={`${selectedClassName()}`}>{children}</Comp>
);

Copy link
Member Author

Choose a reason for hiding this comment

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

코드가 거짓일 때 빈 fragment를 반환함으로서 명시적으로 빈 값을 나타내기 위해서 작성하였습니다.
논리연산자를 사용하면 undefined나 false를 반환하는데 반해 특정 조건에서 아무것도 렌더링 하지 않겠다 라는 의도를 명확히 표현할 수 있다고 생각했습니다.

Comment on lines +14 to +16
.${mobileOnlyClassName} {
display: none !important;
}
Copy link
Member

Choose a reason for hiding this comment

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

P3) !important를 사용하는 대신 쌓임맥락을 고려해서 스타일 우선순위를 높여볼 수도 있을 것 같습니다. 해당 방법이 통하지 않아서 !important 사용하신 걸까요 ??

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 로직은 모든 페이지에서 사용되는 부분입니다. 이것이 서드파트 라이브러리일 수도 있고 모든 부분의 쌓임맥락을 고려할 수는 없기에 !important를 사용하였습니다.

Copy link
Contributor

@moondda moondda left a comment

Choose a reason for hiding this comment

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

깔꼼합니다

const Responsive = ({ children, only, asChild }: ResponsivePropTypes) => {
const [current, setCurrent] = useState<AvailableSize | null>(null);

const Comp = asChild ? Slot : 'div';
Copy link
Contributor

Choose a reason for hiding this comment

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

p5) 굿

Comment on lines +1 to +15
import { createContext } from 'react';

interface ResponsiveContextValue {
mobileOnlyClassName: string;
desktopOnlyClassName: string;
}

export const ResponsiveContext = createContext<ResponsiveContextValue>(
//ResponsiveProvider로 감싸져있지 않은 컴포넌트에서 값을 접근했을 때 또는 Context가 제대로 초기화되지 않았을 때 에러를 발생시켜 디버깅을 쉽게하기 위함
new Proxy({} as ResponsiveContextValue, {
get() {
throw new Error('ResponsiveProvider가 필요합니다.');
},
}),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

p5) 저희는 전체 컴포넌트들이 다 프로바이더로 감싸져있는데 이 디버깅이 필요한 경우가 있나요??

Copy link
Member Author

Choose a reason for hiding this comment

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

누군가 실수로 프로바이더 영역 외에 컴포넌트를 만들어 해당 ResponsiveContext를 사용할 경우 디버깅에 유리합니다. 해당 로직은 에러처리를 용이하기 위해 작성한 로직입니다. 휴먼에러는 언제든 발생할 수 있으니까요!

Copy link
Contributor

@se0jinYoon se0jinYoon left a comment

Choose a reason for hiding this comment

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

lgtm 수고하셨어요!
여기저기 보이는 주석들은 아직 피그마 뷰 완성이 안되어서 그런 거라고 생각하고 코리 따로 안 달아놨어요. 필요없는 것들이라면 머지 전에 제거하셔도 될 것 같슴다~

@@ -38,4 +41,10 @@ const DesktopWrapper = styled.div`
align-items: center;
width: 100%;
scroll-behavior: smooth;

@media ${MOBILE_MEDIA_QUERY} {
/* width: 100%; */
Copy link
Contributor

Choose a reason for hiding this comment

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

p5) 지우기~

`;

const SubText = styled.span<{ isContainPhoto: boolean; isLast: boolean }>`
display: -webkit-box;
Copy link
Contributor

Choose a reason for hiding this comment

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

p5) 이걸로 바꿔준 이유는 뭔가여?

Copy link
Member Author

Choose a reason for hiding this comment

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

블록 컨테이너의 문자열을 지정한 줄 수만큼 제한할 수 있는 -webkit-line-clamp 속성을 사용하기 위해 추가하였습니다!

@ljh0608 ljh0608 merged commit aed1cc8 into develop Nov 17, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
♻️ Refactor 코드 리팩토링 size/l size/l 재훈
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants