[feat] 스터디 목록 페이지 추가 - #63
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Antoliny0919
left a comment
Antoliny0919
left a comment
There was a problem hiding this comment.
하하 드디어 첫 페이지를 만들었습니다 👍
코멘트 한 번 확인해줄래요 ? @DongEun02
| const response = await api.get('/studies/me'); | ||
| return await response.json<{ studies: Study[] }>(); |
There was a problem hiding this comment.
unknown 타입 좁히기가 필요할까요 ?
There was a problem hiding this comment.
음..이 부분은 타입 좁히기라고 보긴 어려울 것 같아요. 타입 좁히기보단 단지 TypeScript한테 이 응답은 Study[]라고 생각해라고 지정해주는 것 뿐이라고 생각해요.
어떤 데이터가 들어올지 모르니까 초기 타입은 unknown으로 선언하고 타입 가드로 타입 좁히기를 하는 방법이 좋아보여요!
There was a problem hiding this comment.
index e38f7b8..02b8468 100644
--- a/frontend/src/features/studies/api.ts
+++ b/frontend/src/features/studies/api.ts
@@ -1,11 +1,56 @@
import api from '../../client';
-import { Study } from './types';
+import { Role, Study } from './types';
+
+const ROLES: readonly Role[] = ['STUDY_LEADER', 'SOME'];
+
+function isRole(value: unknown): value is Role {
+ return ROLES.includes(value as Role);
+}
+
+function isStudy(value: unknown): value is Study {
+ return (
+ typeof value === 'object' &&
+ value !== null &&
+ 'id' in value &&
+ typeof value.id === 'string' &&
+ 'role' in value &&
+ isRole(value.role) &&
+ 'title' in value &&
+ typeof value.title === 'string' &&
+ 'description' in value &&
+ typeof value.description === 'string' &&
+ 'memberCount' in value &&
+ typeof value.memberCount === 'number' &&
+ 'noticeCount' in value &&
+ typeof value.noticeCount === 'number' &&
+ 'assignmentCount' in value &&
+ typeof value.assignmentCount === 'number'
+ );
+}
+
+function isStudiesResponse(value: unknown): value is { studies: Study[] } {
+ return (
+ typeof value === 'object' &&
+ value !== null &&
+ 'studies' in value &&
+ Array.isArray(value.studies) &&
+ value.studies.every(isStudy)
+ );
+}
export async function fetchStudies() {
+ let data: unknown;
+
try {
const response = await api.get('/studies/me');
- return await response.json<{ studies: Study[] }>();
+ data = await response.json();
} catch {
throw new Error('스터디 목록을 불러오는데 실패했습니다.');
}
제너릭을 믿지않고 unknown 타입좁히기를 사용하면 위와 같은 형태가 될거 같아요 !
지금 필드만으로도 위와같은 긴 검사를 하는 코드가 만들어지게 되네요 ..
또 이 부분에 대한 유닛테스트도 필요하구요.
하지만 이 코드가 말한것처럼 무의미하지 않고 필요한 부분같아요.
애초에 제너릭에 선언한 타입은 런타임엔 무의미 하니까요.
그냥 선언일 뿐인거죠.
그래서 실제로 다른 응답이 오게된다면 문제가 발생할거 같아요.
그런의미로 저런 타입가드가 필요한거겠죠.
하지만 지금 당장 위와같은 코드가 추가되고 유닛테스트까지 추가해야하는 비용까지 고려하면 지금당장 고려하는 것보다 급한 기능들 부터 만들고 이후에 천천히 파악하고 고려하면 될거같네요.
흠.. 필요하면 zod를 도입하는게 괜찮을 수 있겠네요.
이 부분은 따로 Issue 만들고 이후에 퀄리티를 높여가는 과정에서 진행하시죠 !
| }, | ||
| ]; | ||
|
|
||
| const STUDIES_URL = 'https://mock.chongchong.com/studies/me'; |
There was a problem hiding this comment.
STUDIES_URL을 다시 선언해야 한다는 점에서 유지보수하기 어려울 수 있겠다는 생각이 들었습니다.
왜냐하면 만약 url이 바뀌게 되면 여러곳을 수정해야 하니까요.
경로가 바뀌면 현재 파일과 api.ts 를 수정해야 하고 도메인이 바뀌면 client와 현재 파일이 수정되어야 합니다.
API URL을 한곳에서 모아서 관리하는게 나을까요 ?
There was a problem hiding this comment.
저는 항상 https://mock.chongchong.com까지를 BASE_URL로 상수화하고, 그 다음부터는 직접 입력해서 사용했었어요. 유지보수를 생각한다면 BASEURL을 전역으로 두고, 각 도메인 별로 상세 URL을 관리하는 것은 어떨까요??
There was a problem hiding this comment.
좋아요 👍
루트에 BASE_URL을 두고 도메인별 API 경로들을 모아두는 파일들을 따로둔다음에 필요한 곳에서 조합해서 사용하시죠 !
| <ContentCard.Title>{study.title}</ContentCard.Title> | ||
| <ContentCard.Trailing> | ||
| <a href=""> | ||
| <img src={rightArrowIcon} alt="" css={{ width: '20px', height: '20px' }} /> |
There was a problem hiding this comment.
인라인 스타일과 외부에서 스타일을 정의해서 가져다 쓰는 방식중 기준이 있으신가요 ?
저는 지금당장은 변할 가능성이 낮고 적은 속성일때는 인라인에 추가하는거 같습니다.
반대로 속성이 많고 변할 가능성이 존재하는 경우에는 따로 정의하는거 같아요. (사실 변할 가능성보다는 속성이 많게되면 자연스럽게 분리되는거 같습니다.)
인라인으로 적용했을때는 타입을 적용하기 어려울거 같습니다.
뭐 <img src={rightArrowIcon} alt="" css={{ width: '20px', height: '20px' } satisfies CSSProperties} /> 처럼 적용은 할 수 있겠지만 이런식으로 인라인에 정의하는 방식이 코드를 보는 입장에서 괜찮은지 모르겠네요.
그렇다고 분리하기에는 또 애매한거 같습니다.
지금처럼 속성이 적고 변할 가능성이 낮은 경우에는 인라인으로 정의하면서 타입을 붙이는게 나을까요 ?
There was a problem hiding this comment.
이건 정답이 없는 것 같아요. 사실 너무 간단한 스타일이라 오히려 코드를 읽는 흐름을 망칠 것 같다는 생각이 들어요.
There was a problem hiding this comment.
인라인을 두는 방식을 선택하면서 타입까지 안전하게 할 수 있는 방법을 고려해 보시죠 ..
There was a problem hiding this comment.
개인적으로 페이지파일을 어디에 두는게 좋을지 고민하다가 그냥 studies 루트에 뒀습니다.
최대한 연관된 컴포넌트와 함께 두고 싶은 마음이 있어요.
예시를 들자면 MyStudies와 StudyList죠.
하지만 만약 둘 컴포넌트중에 재사용하는 컴포넌트가 존재하게 되면 이 위치는 애매하게 됩니다.
1, 2중에서 1에 둬야할지 2에 둬야할지 애매하니까요.
이런 경우를 생각하면 지금처럼 그냥 분리해서 두는게 괜찮겠다는 생각이 듭니다.
물론 현재는 페이지가 하나지만 만약 여러개가 되는 경우에도 괜찮을지는 의문이네요 🤔
혹시 디움은 이 부분에 대해서 어떻게 생각하시나요 ?
우리가 더 응집도 좋고 결합은 낮은 폴더구조를 만들 수 있을까요 ?
There was a problem hiding this comment.
저라면 페이지가 여러 개인 경우가 생기면 pages라는 폴더를 만들어서 그 안에 페이지 파일을 위치시킬 것 같아요.
응집도가 높은 것과 결합도가 낮은 것 모두 중요하지만 폴더 구조에서 모든 것을 챙길 수는 없을 것 같아요...
There was a problem hiding this comment.
좋습니다. 의견을 들을 수 있어서 좋네요.
pages폴더를 주고 페이지 관련된 파일들을 모아두죠 !
| @@ -0,0 +1,11 @@ | |||
| type Role = 'STUDY_LEADER' | 'SOME'; | |||
There was a problem hiding this comment.
지금 당장은 SOME ...
DongEun02
left a comment
DongEun02
left a comment
There was a problem hiding this comment.
고생하셨습니다!
UI
우선 UI에서 제가 표시한 부분 gap을 조금 더 줘도 될 것 같아요. 그리고 버튼 하단에 아이콘 + "리마인드는 총총이 보낼게요" 영역을 중앙 정렬하면 일관성 있는 디자인이 될 것 같습니다.
코멘트와 수정할 부분
이 부분은 코멘트로 남겨놨으니 봐주시면 될 것 같아요!
| const response = await api.get('/studies/me'); | ||
| return await response.json<{ studies: Study[] }>(); |
There was a problem hiding this comment.
음..이 부분은 타입 좁히기라고 보긴 어려울 것 같아요. 타입 좁히기보단 단지 TypeScript한테 이 응답은 Study[]라고 생각해라고 지정해주는 것 뿐이라고 생각해요.
어떤 데이터가 들어올지 모르니까 초기 타입은 unknown으로 선언하고 타입 가드로 타입 좁히기를 하는 방법이 좋아보여요!
| <a href=""> | ||
| <img src={rightArrowIcon} alt="" css={{ width: '20px', height: '20px' }} /> |
There was a problem hiding this comment.
네비게이션이 ArrowIcon에만 적용되어 있는데 모바일 앱을 사용한다고 생각했을 때 Item 영역에 네이게이션을 적용하면 좋을 것 같다는 생각이 들었어요. 제가 사용자였을 때 ArrowIcon을 눌렀을 때만 페이지가 이동하면 불편하다고 느낄 것 같은데 어떻게 생각하시나요??
There was a problem hiding this comment.
좋아요 !
저는 접근성을 고려해서 루트에 두지 않았어요.
스크린리더에서 a태그 내부 요소에 접근할 수 없거든요.
근데 확인해보니 내부 내용이 다 접근은 못하지만 읽히기는 해서 문제없을거 같네요 !
👍👍
| <ContentCard.Title>{study.title}</ContentCard.Title> | ||
| <ContentCard.Trailing> | ||
| <a href=""> | ||
| <img src={rightArrowIcon} alt="" css={{ width: '20px', height: '20px' }} /> |
There was a problem hiding this comment.
이건 정답이 없는 것 같아요. 사실 너무 간단한 스타일이라 오히려 코드를 읽는 흐름을 망칠 것 같다는 생각이 들어요.
| }, | ||
| ]; | ||
|
|
||
| const STUDIES_URL = 'https://mock.chongchong.com/studies/me'; |
There was a problem hiding this comment.
저는 항상 https://mock.chongchong.com까지를 BASE_URL로 상수화하고, 그 다음부터는 직접 입력해서 사용했었어요. 유지보수를 생각한다면 BASEURL을 전역으로 두고, 각 도메인 별로 상세 URL을 관리하는 것은 어떨까요??
There was a problem hiding this comment.
저라면 페이지가 여러 개인 경우가 생기면 pages라는 폴더를 만들어서 그 안에 페이지 파일을 위치시킬 것 같아요.
응집도가 높은 것과 결합도가 낮은 것 모두 중요하지만 폴더 구조에서 모든 것을 챙길 수는 없을 것 같아요...
b8ba903 to
191e527
Compare
굿굿 동은 고마워요. |
2 participants
연관 이슈
Closes #52
To-Be
스터디 목록 페이지를 추가합니다.
이외에 RTL 테스트를 위한 유틸들을 추가했습니다.
해당 부분들은 src/test/render.ts 에 존재합니다.
Tanstack을 사용하는 환경에서 필요한 Provider 를 생성해주는 함수와 응답데이터를 변환하는 함수를 추가했습니다.
스크린샷 (UI 변경 시)
스터디 (O)
스터디 (X)
체크리스트