-
Notifications
You must be signed in to change notification settings - Fork 41
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 : index.html을 기반으로 메인 페이지를 구성합니다. #58
Conversation
src/styles/theme.css.ts
Outdated
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.
이 전역 Theme 내에서 다른 css variables를 설정해서 사용해줄수 있을것 같은데, 일단 전체적인 background 색상만 지정해놓았습니다!
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/utils/generateThemeScript.ts
Outdated
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.
ssr 환경에서 단순히 리액트 훅들을 사용해서 다크모드를 구현하려 하면, 다크모드 상태에서 새로고침을 했을때 흰색 바탕이 일시적으로 깜빡였다가 후에 다크모드가 적용 되는 현상이 발생합니다.
그걸 방지하기 위해 렌더링 되기전에 즉시 실행 함수를 실행시켜주어서 해당 테마를 적용시켜주었습니다.
children: React.ReactNode; | ||
}) { | ||
const currentTheme = getThemeCookieValue(); | ||
const themeInitializerScript = generateThemeScript(currentTheme); |
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.
이거 좋네요 저도 유사하게 개발해보겠습니다 감사합니다
지금 다시 보니 base branch만 수정해주시면 좋을 것 같습니다 ㅎㅎ main으로 향해 있군요 |
if (!currentTheme) { | ||
currentTheme = "light"; | ||
} |
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.
currentThemeObj 가 옵셔널 체이닝 되어 상단 ||
에 걸릴 것 같은데 조건문이 있어야 하나용?
export const lightVars = createGlobalTheme(":root", { | ||
colorBackground: "#fff", | ||
text_color: "#000", | ||
}); |
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.
":root"
는 추후에 variables 객체 모아놓은 코드로 분리할 수도 있겠네요! 역시 VE는 직관적입니다 😌
const popularTags = countArticleTags({ articles }); | ||
|
||
if (!popularTags) { | ||
return []; | ||
} else { | ||
return popularTags; | ||
} |
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.
const popularTags = countArticleTags({ articles }); | |
if (!popularTags) { | |
return []; | |
} else { | |
return popularTags; | |
} | |
const popularTags = countArticleTags({ articles }) || []; | |
return popularTags; |
요건 어떨까요?
그리고 상단의 코드 라인들이 다 한 칸씩 떨어져 있는데 조건문이냐, 같은 관심사냐, 그루핑된 맥락이냐에 따라 합쳐도 좋을 것 같습니다용
|
||
const res = await fetch(endPoint); | ||
|
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.
이렇게 사용하는 데이터 패칭 코드 라인은 추후 리팩토링 되는 것이겠죠?!
const getArticlesEndPoint = `${process.env.NEXT_PUBLIC_GET_ARTICLES_END_POINT}?offset=0&limit=9`; | ||
|
||
if (!getArticlesEndPoint) { | ||
throw new Error("엔드포인트를 확인해주세요! "); | ||
} |
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.
템플릿 리터럴로 감쌌는데 부정문이 걸릴 일이 있나요? 😳
const httpAbortCtrl = new AbortController(); | ||
activeHttpRequests.current.push(httpAbortCtrl); |
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.
보통은 useEffect에서 패치 핸들링할 때 자주 활용하는데... 신선한 방식이네요 👀
headers: Record<string, string> = {} | ||
): Promise<any> => { |
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.
any 타입을 제거해볼 수 있을 것 같은데요?!
export const useDarkMode = (currentTheme: string) => { | ||
const [darkTheme, setDarkTheme] = useState<boolean>(currentTheme === "dark"); |
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.
어디서 본 것 같은 코드가....
useTheme도 비슷했던 맥락인거 같네요!
type Children = { | ||
children: React.ReactNode; | ||
}; |
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.
요렇게 사용하실거라면 PropsWithChildren 타입은 어떠신가요?
} from "@/styles/popular_tags.css"; | ||
import { color_state } from "@/styles/container.css"; | ||
|
||
type tagListType = { |
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.
컨벤션...! 변수인줄 알았네요 🫠
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.
리뷰들 다 확인했습니다! 한번 말씀 해주신대로 수정해보겠습니다 :)
하나하나 확인해주시고 말씀주셔서 감사드려요 🤩
📖 작업 배경 : 🌟 FEATURE: [2 Week] 메인 페이지를 개발합니다. #47
🛠️ 구현 내용
💡 참고사항
🖼️ 스크린샷