-
Notifications
You must be signed in to change notification settings - Fork 1
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] LogPage, HistoryPage 퍼블리싱 및 리스트 컴포넌트 작업 #47
base: develop
Are you sure you want to change the base?
Conversation
------------------------------------------------------ ------------------------------------------------------
------------------------------------------------------ ------------------------------------------------------
… into design/log-page-37
------------------------------------------------------ ------------------------------------------------------
------------------------------------------------------ ------------------------------------------------------
------------------------------------------------------ ------------------------------------------------------
<span className={styles.headerTitle}>차량번호</span> | ||
<span className={styles.headerTitle}>부서명</span> | ||
<span className={styles.headerTitle}>이름</span> | ||
<span className={styles.headerTitle}>운행일수</span> | ||
<span className={styles.headerTitle}>평균운행거리</span> | ||
<span className={styles.headerTitle}>평균운행시간</span> | ||
<span className={styles.headerTitle}>총운행거리</span> | ||
<span className={styles.headerTitle}>운행률</span> | ||
<span className={styles.headerIcon}>icon</span> |
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.
span 태그의 경우, 인라인 요소이라 구조적 레이아웃에는 div 태그가 더 적절해보입니다.
PR에 작성해주신 내용을 보니, 이 부분은 선택적으로 하셔도 좋을 것 같습니다:)
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.
span
보다는 div
가 구조적으로 더 명확한 것 같네요! 수정하겠습니다:)
export const headerIcon = style({ | ||
width: '24px', | ||
height: '24px', | ||
visibility: 'hidden', |
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.
단순 공간을 위해 콘텐츠를 작성하고 visibility: 'hidden'
를 사용하기 보다, <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.
오오 굿굿
interface HistoryItemProps { | ||
data: VehicleHistoryModel | ||
} |
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이므로, ListItemProps
가 적절해보입니다.
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.
...정신 차릴게요!
<span className={styles.itemContent}>{averageDrivingTime}분</span> | ||
<span className={styles.itemContent}>{totalDrivingDistance}km</span> | ||
<span className={styles.itemContent}>{drivingRate}%</span> | ||
<div className={styles.icon}>{icon}</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.
여기서 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.
div
통일 절대 지켜
app/(dashboard)/history/page.tsx
Outdated
<div className={styles.componentsWrapper}> | ||
<div className={styles.searchInputWrapper}> |
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.
div가 중첩이 되어있는데 componentWrapper와 searchInputWrapper를 구분하신 이유가 있으실까요?
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.
LogPage랑 헷갈렸습니다..! 이 부분은 컴포넌트로 따로 만들어 관리하도록 하겠습니다!
app/(dashboard)/history/page.tsx
Outdated
<div className={styles.listWrapper}> | ||
<ListHeader /> | ||
{mockHistoryData.map((history) => ( | ||
<ListItem key={history.id} data={history} /> | ||
))} | ||
</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.
페이지에서 독립된 컴포넌트로 제작해보는 것도 좋아보입니다. 물론 선택입니다:)
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 * as styles from './styles.css' | ||
|
||
interface ListItemProps { |
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.
여기처럼 위 History도 컨벤션대로 수정해주세요~
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.
넵!
app/(dashboard)/log/page.tsx
Outdated
<div className={styles.componentsWrapper}> | ||
<div className={styles.buttonWrapper}> | ||
<RoundButton color='secondary' onClick={() => {}} size='small'> | ||
등록 | ||
</RoundButton> | ||
</div> | ||
<div className={styles.searchInputWrapper}> | ||
<SearchInput icon='/icons/search-icon.svg' /> | ||
</div> | ||
</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.
버튼과 인풋의 wrapper를 생성해 각각 레이아웃을 구성하기 보다, 두 개를 하나의 레이아웃으로 묶어 위치를 조정하시는 게 더 편하실 겁니다. 두 개를 묶은 레이아웃을 position: absolute
그리고 그 안에서 displex: flex
와 gap
으로 스타일링해보심 좋을 것 같아요!
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의 신..ㄷㄷ
@@ -13,7 +13,7 @@ export const shape = styleVariants({ | |||
[BADGE_SHAPE.RECTANGLE]: { | |||
paddingLeft: '12px', | |||
paddingRight: '12px', | |||
fontSize: styles.fontSizes.small, | |||
fontSize: styles.fontSizes.medium, |
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.
기본 폰트가 16px이므로, 해당 내용은 삭제해도 좋을 것 같습니다:)
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.
네에🐰🎀
small: { | ||
width: '76px', | ||
height: '48px', | ||
backgroundColor: styles.colors.white, | ||
backgroundColor: styles.colors.primary, | ||
border: `1px solid ${styles.colors.gray[200]}`, | ||
boxShadow: `0px 4px 4px 0px ${styles.colors.shadow[100]}`, | ||
color: styles.colors.white, | ||
}, | ||
large: { | ||
color: styles.colors.primary, | ||
height: '60px', | ||
}, | ||
}, | ||
color: { | ||
primary: { | ||
color: styles.colors.primary, | ||
}, | ||
secondary: { | ||
color: styles.colors.black, | ||
padding: '16px 24px', |
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.
size로 구분하신 스타일에 color 스타일링이 들어가고 있습니다. 이는 위 color로 구분한 것과 같이 쓸 때 문제가 될 수도 있다고 생각합니다. 만약 각각 디자인 버튼을 구분하고 싶으시면 recipe보다 styleVariant로 작성하시면 조금 더 편하실 수도 있어요:)
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가 익숙하지 않아 recipe
와 styleVariant
의 차이점을 잘 몰랐는데 덕분에 어떻게 구분하면 좋을지 알게 됐네요! 감사합니다!
… into design/log-page-37
------------------------------------------------------ ------------------------------------------------------
- 기존에 분리되어 있던 ListHeader 컴포넌트를 하나로 병합 - headerTitles props로 헤더 제목 리스트를 받도록 개선 - 컴포넌트 재사용성 향상 및 코드 중복 제거 ------------------------------------------------------ ------------------------------------------------------
------------------------------------------------------ ------------------------------------------------------
------------------------------------------------------ ------------------------------------------------------
------------------------------------------------------ ------------------------------------------------------
- props로 전달받던 icon을 컴포넌트 내부로 이동 - 컴포넌트 구조 단순화 ------------------------------------------------------ ------------------------------------------------------
------------------------------------------------------ ------------------------------------------------------
------------------------------------------------------ ------------------------------------------------------
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.
고생하셨습니다!
🚀 풀 리퀘스트 제안
closes #36
closes #37
📋 작업 내용
차량 리스트 컴포넌트 구현 및 구조 변경
ListHeader
,ListItem
컴포넌트 생성VehicleList
페이지 경로 삭제페이지 퍼블리싱 및 스타일링
LogPage
및HistoryPage
레이아웃 퍼블리싱SquareButton
패딩값 수정theme.css
에opacity
상수 추가 및 컴포넌트 적용데이터 처리
🔧 변경 사항
📸 스크린샷
📄 기타
💻 코드 설명
🌟
ListHeader
를 하나의 컴포넌트가 아닌 두개로 분리하고 하드코딩한 이유:🌟
ListItem
span을 사용한 이유:🖱️ Commits 참고
리베이스를 잘못해서 커밋이 뒤섞이는 이슈 발생.. 커밋단위는 전에 올렸던거 참고해주세요:)
📍 2024/01/09 수정내용
기존에 설계했던 그림은 두 개의 컴포넌트를 하나의 컴포넌트로 만들어 사용하고 싶었는데
능력+지식 부족으로 두 개의 컴포넌트를 하나에 구겨 넣지도 못하는(?) 설계가 되어버렸네요🤯
ListItem
컴포넌트를 각각 페이지의 컴포넌트로 분리하고,VehicleLogModel
과VehicleHistoryModel
를 생성하여types
폴더로 분리했습니다.📍 2024/01/10 수정내용
🌟 드디어
ListHeader
컴포넌트를 하나로 통일했습니다:)🌟
ListItem
컴포넌트는 변경사항X구조 분해 할당 사용 이유:
배열로 변환하여 map을 사용하는 방식: