셔틀버스, 등하교버스, 생협 업데이트 페이지 추가 #113
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
kang-s-h
left a comment
kang-s-h
left a comment
There was a problem hiding this comment.
전체적으로 수정하였습니다
파일 선택 후 취소를 누르거나 이관을 누른 이후에 모든 버튼이 disable 상태가 됩니다.
의도하신 로직일까요..?!
→네 스프린트 안에 이관하거나 취소하는 경우 학기선택 뺴고 모든버튼을 비활성화 하는걸로 알고 있습니다.
어드민 UI 작업이 처음이셨을 텐데, 정말 고생 많으셨습니다.
아래 코멘트와는 별개로, 전반적으로 수정해주시면 좋을 것 같은 부분을 추가로 정리드릴게요.
- 파일 구조 및 네이밍 관련
어드민에서는 styled-components를 사용하고 있어, 페이지 파일을 단순히 index.tsx로 두기보다는 어떤 페이지인지 드러나는 이름으로 작성해주시면 좋을 것 같습니다.
기존 코드들을 보면 단일 파일만 존재할 때는 index.tsx를 쓰기도 하지만, 여러 파일이 있는 경우에는 페이지 목적을 명확히 나타내는 이름을 사용하는 편이에요.
또한 스타일 파일은 index.style.tsx 형태로 쓰지 않고, 공통적으로 XXX.style.tsx 형태를 사용하고 있으니 기존 패턴과 동일하게 맞춰주시면 좋을 것 같아요
- Preview 컴포넌트 depth 관련
Preview 컴포넌트 구조가 전체적으로 깊어지는 경향이 있어 가독성이 다소 떨어지는 것 같아요.
공통 레이아웃을 분리하거나, 테이블 헤더·컨텐츠를 각각 컴포넌트로 분리하거나, 기존에 있는 커스텀 Table 컴포넌트를 활용하는 방식으로 개선할 수 있을 것 같고 이런 방식으로 리팩터링하면 코드 구조도 더 단순해지고 유지 보수도 쉬워질 것 같아요.
- PR 단위 관련
- 전체 코드가 약 1600줄 정도 되는 큰 PR이었는데, 트랙장님께서도 가능하면 500줄 내외로 맞추자고 하신 점, 그리고 일반적인 PR 크기를 고려하면 다소 큰 편인 것 같습니다. 페이지 별로 나누어 올려주셨다면 도메인별로 분리도 되고 리뷰도 훨씬 수월했을 것 같아서 다음 작업하실 때는 고려해주시면 좋을 것 같습니다!
고생하셨습니다!
→1. 이부분 신경쓰려고 했는데 index부분은 차마 신경을 못썼네요.. 수정하곘습니다.
2. 맞는것 같네요 이부분은 한번 고민 해보겠습니다.
3. pr올리는 당시에는 기억을 못했는데 말씀해주시니 기억이 나네요. 다음부턴 사이즈 생각해서 pr올리겠습니다.
| const ROUTE_TYPE_COLORS: Record<string, string> = { | ||
| 주중: '#ffb443', | ||
| 주말: '#34adff', | ||
| 순환: '#4ed92c', | ||
| } as const; |
| padding-left: 24px; | ||
| padding-right: 12px; | ||
| min-height: 54px; | ||
| gap: 8px; //여기 다름 |
| export const Container = styled.div` | ||
| margin-left:20px; | ||
| `; |
| export const FileSettingWrapper = styled.div` | ||
| width: 70%; | ||
| border: solid #dfdfdfff; | ||
| border-width: 1px 0px; | ||
| padding : 10px 0px; | ||
| display: flex; | ||
| justify-content: space-between; | ||
| `; | ||
|
|
||
| export const FileDataWrapper = styled.div` | ||
| width: 100%; | ||
| height: 70vh; | ||
| border: 1px solid #d3d3d3ff; | ||
| display: flex; | ||
| justify-content:center; | ||
| align-items: center; | ||
| `; | ||
|
|
||
| export const TextBox = styled.div` | ||
| line-height: 32px; | ||
| `; |
src/App.tsx
Outdated
| <Route path="/shuttleBus-update" element={<ShuttleBusUpdate />} /> | ||
| <Route path="/commutingBus-update" element={<CommutingBusUpdate />} /> | ||
| <Route path="/coopshop-update" element={<CoopshopUpdate />} /> | ||
| <Route path="/coopShop-update" element={<CoopShopUpdate />} /> |
There was a problem hiding this comment.
BCSD에서는 url 경로에 대문자를 사용 안하는 것으로 알고 있어요
윈도우/ 리눅스 유닉스 별로 url에서 대소문자 구분 여부도 달라서 소문자로 통일해주시는게 좋을 것 같습니다
update가 문제라면 개인적으로 update/force, update/shuttle-bus 이런식으로 공통 라우트 주고 아래에서 처리하는게 좋을 것 같네요
What is this PR? 🔍
기능 : 생협, 셔틀버스, 등하교버스 업데이트 페이지를 추가하였습니다.
이 페이지를 통해 해당 데이터를 가지고 있는 엑셀을 미리보기 할 수 있고
바로 데이터를 이관할 수 있습니다.
issue : 셔틀버스, 등하교버스, 생협 업데이트 추가 #112