-
Notifications
You must be signed in to change notification settings - Fork 3
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: add dev-test draft #22
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
작업 깔끔하게 잘해주고 계시네요 👍 코드 레벨에서 몇가지 리뷰했습니다!
그리고 Button
, token.ts
등 공통 디자인 토큰 관련된 작업은 다른 PR로 나누어 주실 수 있을까요? 커뮤니티팀 고유의 작업과 분명히 분리된 PR로 올라오는 게 좋을 것 같아서요!
src/components/comuunity/index.ts
Outdated
import DevTestPage from './playground/DevTest'; | ||
|
||
export { DevTestPage }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍎 폴더 이름 community인데 comuunity로 오타났어요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헉 수정하겠습니다! 감사해요!!!
import { COLORS } from '../common/token'; | ||
|
||
interface Props { | ||
setStage: Dispatch<SetStateAction<number>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥝 setter 자체를 넘겨주면 외부 로직에 너무 의존성이 높아질 것 같은데, 단순히 '이 스텝이 끝날 때 실행된다'는 의미로 onFinish
또는 onEnd
느낌으로 네이밍을 한 뒤에, 타입을 () => void
로 단순히 정의하면 어떨까요? 그리고 외부에서는 이런 식으로 쓰면 될 것 같아요.
const goToNextStep = () => {
setState(prev => prev + 1);
};
return (
<PlayPage onEnd={goToNextStep} />
);
이런 느낌으로 하면 더 유연하게 짤 수 있지 않을까요?
그리고 이렇게 한다면 모든 step에 쓰이는 컴포넌트들이 동일한 interface를 가지도록 할 수 있을 것 같아요. 예를 들어서
// src/components/community/playground/DevTest/types.ts
export interface StepProps {
onEnd?: () => void;
}
// src/components/community/playground/DevTest/PlayPage.tsx
import type { StepProps } from './types';
interface Props extends StepProps {}
이렇게 짜도 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오홍...!! 뭔가 함수 넘겨주기보다 setter 넘겨주는게 편하다고 생각해서 아무 생각없이 이렇게 해왔는데 인터페이스도 통일할 수 있고 의존성 문제도 해결할 수 있겠네요...
수정하겠습니당!!
setStage: Dispatch<SetStateAction<number>>; | ||
} | ||
|
||
const PlayPage = (props: Props) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 props를 spread하지 않는 이유가 따로 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ㅋㅋㅋㅋㅋ앗 props가 하나라 귀..찮아서 안했던건데 들켰네효...ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ
수정하겠습니당!
<style jsx>{` | ||
.devtest__play-page { | ||
width: 100%; | ||
height: 100%; | ||
display: flex; | ||
flex-direction: column; | ||
align-items: center; | ||
justify-content: center; | ||
gap: 50px; | ||
} | ||
.devtest__play-page--question, | ||
.devtest__play-page--select { | ||
width: 100%; | ||
display: flex; | ||
flex-direction: column; | ||
align-items: center; | ||
gap: 15px; | ||
} | ||
`}</style> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍌 저희는 이번 프로젝트에서 스타일링 라이브러리로 vanilla-extract
를 사용하기로 했는데, 혹시 styled-jsx를 쓰신 이유가 따로 없다면 vanilla-extract
로 바꿔보면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵!! 변경하겠습니당
{stage === 0 ? ( | ||
<StartPage setStage={setStage} /> | ||
) : ( | ||
<PlayPage setStage={setStage} /> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥝 stage
가 앞으로도 0
또는 1
만 있을 거라는 보장이 없으니 조금 더 확장성 있게 설계하면 어떨까요?
const STAGES = [
StartPage,
PlayPage,
] as const;
const DevTestPage = () => {
// ...
const CurrentPage = STAGES[stage];
return (
<div className="devtest__page">
<Wrapper topColor="#00A4CA" bottomColor="#58C4C4">
<CurrentPage setStage={setStage} />
{/* ... */}
);
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오홍... 훨씬 직관적이네요! 감사합니다~!
@@ -0,0 +1,54 @@ | |||
import { TEXT_STYLE_BUTTON_PC, TEXT_STYLE_BUTTON_MOBILE } from '../token'; | |||
|
|||
interface Props { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍌 어차피 내부에서 <button>
을 직접적으로 사용하니, <button>
이 받을 수 있는 모든 prop을 허용해도 되지 않을까요?
interface Props extends React.ComponentProps<'button'> {
backgroundColor: string;
color: string;
title: string;
}
const Button = ({ backgroundColor, color, title, ...buttonProps }: Props) => {
return (
<button {...buttonProps}>
{/* ... */}
);
};
이렇게 하면 onClickHandler
도 생략할 수 있어요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 헐... 너무 좋은 것 같아요.... 대박
감사합니다!
export const TEXT_STYLE_TITLE_PC = { | ||
title: { | ||
fontSize: 36, | ||
fontWeight: 900, | ||
}, | ||
subtitle1: { | ||
fontSize: 30, | ||
fontWeight: 800, | ||
}, | ||
subtitle2B: { | ||
fontSize: 22, | ||
fontWeight: 700, | ||
}, | ||
subtitle2R: { | ||
fontSize: 22, | ||
fontWeight: 500, | ||
}, | ||
subtitle3: { | ||
fontSize: 20, | ||
fontWeight: 700, | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥝 vanilla-extract
를 쓰면 이런 식으로 media query까지 미리 처리해 놓을 수 있어요!
export const text = styleVariants({
title: {
fontSize: 36,
fontWeight: 900,
'@media': {
'screen and (max-width: 500px)': {
fontSize: 30,
fontWeight: 900,
},
},
},
// ...
};
그리고 breakpoint도 상수로 관리해 놓으면 편리해요!
const BREAKPOINTS = [500, 800] as const;
const MEDIA_QUERY = {
pc: `screen and (min-width: ${BREAKPOINTS[1]}px)`,
mobile: `screen and (max-width: ${BREAKPOINTS[0]}px)`,
} as const;
export const text = styleVariants({
title: {
fontSize: 36,
fontWeight: 900,
'@media': {
[MEDIA_QUERY.mobile]: {
fontSize: 30,
fontWeight: 900,
},
},
},
// ...
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오오옹오오 대박...!!!!!
<link | ||
href="https://cdn.jsdelivr.net/gh/sunn-us/SUIT/fonts/variable/woff2/SUIT-Variable.css" | ||
rel="stylesheet" | ||
type="text/css" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍌 외부 CDN을 이용하면 폰트 로딩이 조금 느릴 수 있는데, @next/font/local
을 이용해 보면 어떨까요? (https://nextjs.org/docs/basic-features/font-optimization#local-fonts)
/** 임시 이미지 리소스 -- 커뮤니티팀 | ||
* 임시로 제(이준규) S3버킷에 업로드해서 사용하고 있어요. | ||
* 이미지 업로드 필요하면 말씀해주세요! | ||
*/ | ||
|
||
export const devTestLogo = | ||
'https://horosocular.s3.ap-northeast-1.amazonaws.com/res1.svg'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 로컬 이미지가 아니라 외부에 업로드된 이미지를 사용하려는 이유가 있을까요?
아 그리고 혹시 이미지 업로드가 필요하시면 저희 DB로 쓰고 있는 Supabase에서 가능하니 말씀해 주세요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 그냥 프로젝트 무거워질까봐 그냥 가져와서 쓰려고 했었습니다...!! 상관이 없는건가요?!
넵!!
@@ -21,7 +21,8 @@ | |||
"@/logics/*": ["src/logics/*"], | |||
"@/constants/*": ["src/constants/*"], | |||
"@/assets/*": ["src/assets/*"], | |||
"@/lib/*": ["lib/*"] | |||
"@/lib/*": ["lib/*"], | |||
"@/resources/*": ["src/resources/*"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 src/resources
의 쓰임에 대해서 정리해서 저희 Notion 페이지의 wiki에 올려주실 수 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 정리해서 올려두겠습니다!
onClickHandler={() => props.setStage((prev) => prev + 1)} | ||
/> | ||
<span>GDSC Soongsil Univ.</span> | ||
<img src={devTestLogo} alt="devtest-logo" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍌 앗 리뷰하다가 깜빡했는데, 그냥 img
말고 next/image
의 <Image>
를 쓰면 좋을 것 같아요! 이건 제가 세팅만 해놓고 테스트를 못해서 궁금해서 여쭈어 보는 건데, 혹시 이 부분에서 eslint warning 떴나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 헐 Image 까먹고있었어요...ㅋㅋㅋㅋㅋㅋ 수정하겠씁니다!
아뇨 별도의 문제는 없었습니다!!
Issue
Resolves #20
Description
2022-02-05
코드 리뷰를 위한 Draft PR 입니다!
현재 간단한 재사용 컴포넌트와 함께, 결과 페이지를 제외한 테스트 페이지를 제작하였습니다.
Check List
main
브랜치의 최신 상태를 반영하고 있는지 확인