-
Notifications
You must be signed in to change notification settings - Fork 0
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
Panda CSS 의존성 제거 및 기존 코드 수정 #50
Conversation
}, | ||
"dependencies": { | ||
"@tanstack/react-query": "^5.59.15", |
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.
#35 (comment) 를 참고하여 Tanstack Query도 아직 쓰는 곳이 없길래 우선 제거하였습니다.
필요한 분이 나중에 언제든지 추가할 수 있습니다.
.certificate { | ||
footer { | ||
@media print { | ||
display: none; | ||
} | ||
} | ||
} |
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.
CSS Nesting과 같은 Modern CSS 문법을 잘 활용하면 CSS Modules를 쓰더라도 괜찮은 개발자 경험을 할 수 있습니다. 무엇보다 옛날처럼 복잡하게 다수의 스타일을 임포트해서 여러 HTML 요소에 달아주지 않아서 되고요, 타입 선택자를 사용할 수 있기 때문에 일일이 클래스 네이밍을 해줘야하는 스트레스도 줄일 수 있습니다. 혹시 CSS Nesting이 생소하시다면 이 블로그 포스팅을 읽어보시기 바랍니다: https://www.daleseo.com/css-nesting/
@@ -9,14 +11,14 @@ export default function Leaderboard() { | |||
]; | |||
|
|||
return ( | |||
<div> |
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.
<Certificate/>
과 <Progress/>
는 최상위 요소로 <main>
요소를 쓰는데 이 컴포넌트만 <div>
요소를 쓰고 있어서 통일하였습니다.
<h1>Leaderboard</h1> | ||
|
||
<section aria-labelledby="leaderboard"> | ||
<article aria-labelledby="leaderboard"> |
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.
각 멤버 카드가 독립적인 컨텐츠이기 때문에 <article>
요소가 더 적합한 semantic 요소라고 판단하였습니다.
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.
흠 이 부분에 대해서 section으로 할 지 article로 할 지 고민했는데 피드백 감사합니다!
@@ -1 +1,82 @@ | |||
@layer reset, base, tokens, recipes, utilities; | |||
:root { |
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.
https://github.com/DaleStudy/leetcode-website/blob/main/global-styles.css 의 내용을 그대로 복사해왔습니다. 기존 웹사이트와 일관적인 스타일을 하시는데 도움이 될 거라 생각합니다.
@@ -9,14 +11,14 @@ export default function Leaderboard() { | |||
]; | |||
|
|||
return ( | |||
<div> | |||
<main className={styles.leaderboard}> | |||
<h1>Leaderboard</h1> |
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.
최상위 요소를 main으로 바꾼다면 #17 (comment) 에서 말씀하시 것처럼 전체 페이지가 main에 들어갈 수 있으니 빼는게 좋을까요?
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.
Good point! 제가 후딱 코드를 고치느라 일관성만 생각하고 더 큰 그림을 보지 못했습니다. 이 부분 나중에 <Layout/>
컴포넌트가 도입될 때 팀 차원에서 다시 한 번 검토하면 좋을 것 같습니다. 지금은 우선 각 페이지별로 <h1>
요소가 있는 정도 만으로 만족해도 될 것 같습니다.
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.
수고하셨습니다!
추가로 tsconfig.app.json 파일에 include 속성에 panda css 관련 모듈폴더가 있는거 같은데 제거해주세요 :)
프로젝트에서 Panda CSS 의존성을 제거하고 기존 코드를 CSS Modules를 사용해서 스타일하도록 수정하였습니다. #47 에서 Panda CSS를 쓰지 않는 것으로 최종 합의가 되면 병합하려고 합니다.
테스트
체크리스트