전남대_14조_코드리뷰_4주차 #14

iam454 · 2023-10-01T11:14:15Z

안녕하세요, 멘토님!
연휴는 잘 보내셨나요? 🌰
코드 리뷰를 부탁드리게 된 14조의 FE 테크리더, 서완석입니다.
멘토님의 리뷰는 큰 도움이 될 것이며, 더 나은 개발자로 성장하는 데 기여할 것입니다.
아낌없는 조언 부탁드립니다. 감사합니다.

이번 주는 개발 첫 주차로,
개발 환경 설정과 공통 컴포넌트 생성을 목표로 한 주를 보냈습니다.
수행 내용은 다음과 같습니다.

✅ 산출물

민주
- 게시물 Skeleton UI 생성

건형
- 모달 생성

완석
- 개발 환경 설정
- 스타일 초기화 및 폰트, 색상 결정
- 페이지 경로 설정 및 내비게이션 바 생성
- 공통 컴포넌트 생성
- 인기 페이지 UI 임시 생성

❓ 질문

아래의 질문들은 리드미에 동일하게 작성되어 있습니다.

🍪 건형, GhoRid

컴포넌트를 만들 때, 재사용성을 고려하면 컴포넌트에 받을 props가 너무 많아지는 걸 느꼈습니다. 예를 들어, 모달창 하나를 만드려면 모달창을 열고 닫는 함수, 모달창이 열려있는 상태, 모달창 텍스트, 버튼 함수, 버튼에 들어갈 텍스트(버튼이 2개면 두 개), 버튼 색상, 버튼 아이콘 등... 재사용성을 확보하려면 전달할 props가 워낙 많아지니 이런저런 상황에 활용할 수 있도록 만든 컴포넌트를 처음부터 알아서 만드는 느낌이 강해지는 터라 이에 대해 헷갈립니다. 커스텀을 어디까지 허용해야 할까요?
현재 모달은 modalType이라는 변수에 따라 switch문으로 색상 및 텍스트를 변경할 수 있게 했습니다. modalType만 지정하여 props에 넘겨주면 미리 지정한 switch를 통해 알아서 스타일링이 바뀌게 됩니다. 새로운 상황이 생기면 switch문에 case를 추가하여 처리할 수 있습니다. case를 추가하여 스타일링을 확장하도록 하는 방식은 어떻게 생각하시나요? (코드 리뷰 후 이 방식이 안 좋다는 걸 느껴서 현재는 색상, 텍스트 등을 props로 받아서 직접 지정할 수 있도록 바꿀 예정입니다.)
개발 첫 주를 진행하면서 코드 리뷰의 중요성을 몸소 깨달았습니다. 자주 소통하지 않으면 방향성이 틀어지는 걸 늦게 캐치하여 큰 수정이 필요하게 됩니다. 현업에서는 코드 리뷰의 주기가 어떻게 되나요?

🍪 완석, iam454

1️⃣ 코드 리뷰에 대한 조언을 여쭙고자 합니다.

저희 팀은 weekly 브랜치에 PR을 보내고 1명 이상의 리뷰 후 merge하는 방식을 채택했습니다. 놓친 부분을 리뷰를 통해 잡거나 직접 개발하지 않은 부분도 어느 정도 이해할 수 있게 되어서 장점을 느꼈지만, 단점 역시 느끼고 있습니다. 개발 첫 주동안 느낀 불편한 점은 다음과 같습니다.

같이 배우는 입장이기 때문에, 누가 더 좋은 방향성을 가진 코드인지 판단하기 어렵습니다.
리뷰의 과정이 감정을 상하게 할 수도 있을 것 같아 굉장히 조심스럽습니다.
UI 관련 내용은 merge 이전에 화면으로 볼 수 없습니다.

코드 리뷰는 적절하게 이루어지고 있는지(예시1, 예시2), 불편점을 완화해줄 꿀팁이 있는지 궁금합니다.

2️⃣ 디렉토리 구조에 대한 생각이 궁금합니다.

이 프로젝트는 다음과 같은 구조로 수행될 것 같습니다. 정답이 있는 문제는 아니겠지만 현업에서 자주 사용되는 방식이 있는지, 멘토님이 추천하시는 어떤 방식이 있는지 궁금합니다.

my-app
├── node_modules
├── public
│   ├── index.html
│   ├── favicon.ico
│   └── manifest.json
├── src
│   ├── api
│   │   ├── index.js
│   │   └── ... (다른 API 관련 파일들)
│   ├── components
│   │   ├── CommonComponent1
│   │   ├── CommonComponent2
│   │   └── ... (다른 공통 컴포넌트들)
│   ├── lib
│   │   ├── index.js
│   │   └── ... (다른 라이브러리 관련 파일들)
│   ├── pages
│   │   ├── Page1
│   │   │   ├── components
│   │   │   │   ├── Page1Component1
│   │   │   │   ├── Page1Component2
│   │   │   │   └── ... (다른 페이지 1의 컴포넌트들)
│   │   │   └── Page1.js (or Page1.jsx)
│   │   ├── Page2
│   │   │   ├── components
│   │   │   │   ├── Page2Component1
│   │   │   │   ├── Page2Component2
│   │   │   │   └── ... (다른 페이지 2의 컴포넌트들)
│   │   │   └── Page2.js (or Page2.jsx)
│   │   └── ... (다른 페이지들)
│   ├── utils
│   │   ├── index.js
│   │   └── ... (다른 유틸리티 함수들)
│   ├── App.js
│   ├── index.js
│   └── ...
├── package.json
├── package-lock.json
└── ...

3️⃣ 이미지 파일 관리는 어디서 하는게 좋나요?(public vs src/assets)

이미지 파일은 public 폴더 혹은 src 폴더의 하위로 관리하는 것 같습니다(구글링). 그런데 제 수준에서는 어떤 폴더를 선택하는 것이 좋은지 잘 와닿지가 않습니다. 멘토님께서는 어떤 기준으로 어떤 방식을 선택하여 이미지 파일을 관리하나요? 추천해주시는 방식이 있나요?(아이콘(svg)과 로고(png or svg) 정도가 관리될 것 같습니다.)

4️⃣ 현업에서 라이브러리는 자주 사용되나요? 라이브러리의 선택 기준에는 어떤 것이 있나요?

❗️ 추가로

모달을 직접 구현하는 것보다 react-modal 라이브러리를 사용하는 것이 이점이 더 많을 것 같아서 라이브러리를 사용하기로 결정했습니다. 그럼에도 구현된 모달이 적절한지 평가가 궁금합니다!

weekly -> develop 풀리퀘스트 테스트

App.css index.css logo.svg 삭제

reset css pretendard 글꼴 적용

color값 생성

390*844 레이아웃 페이지 경로는 수정 예정

fork해서 pr 보내는 테스트 -> merge까지 진행

fork해서 PR하기

현재 위치에 따라 내비게이션 색상 변경 기능 생성

테스트용

가짜 데이터 사용

가짜 데이터 사용 실제 데이터 페칭 때 더 봐야할 듯

모바일 환경에 대응할 수 있도록 변경, 인기 페이지 레이아웃 임시 생성, 로컬 테스트용 라이브러리 localtunnel 설치

중앙 좋아요 애니메이션 구현은 안 된 상태 Post 중복된 코드 줄이는 법 생각해야함

chore: swiper 설치

좋아요 애니메이션 생성 및 Post.jsx 수정

Swiper 설치

더블 클릭시 중앙에 좋아요 애니메이션 생성

마이페이지 틀 제작

설정 페이지 UI 생성 + 컨플릭트 해결

pause/resume 가능 time progress bar 현재는 5초로 지정 클릭/더블클릭 이벤트 구분해야함 react-modal overlay click issue

홈 페이지 Swiper 적용

iam454 · 2023-10-08T06:34:56Z

5주차 PR 보내기 위해 merge하겠습니다!

chimimode · 2023-10-10T15:36:11Z

src/pages/HomePage/HomePage.jsx

+            <SwiperSlide key={post.id} onClick={handleSwiperClick}>
+              <Post.Home
+                image={post.image}
+                info={<PostInfos name={post.name} hashtags={post.hashtags} />}


아래 스크린샷 처럼 info에 들어가는 텍스트 정보들의 길이가 길어지는 경우 말줄임(...) 표시나 확장할 수 있도록 액션이 있으면 좋을 것 같습니다.!

좋은 말씀 감사합니다.
이에 대한 다른 방법으로 등록할 수 있는 텍스트 정보들의 길이를 제한하려고 합니다!

그렇지만 멘토님의 제안이 더 좋다고 느끼고 있어서..
개발 기간이 여유가 된다면 그렇게 발전시키겠습니다!

chimimode · 2023-10-10T15:39:45Z

src/components/Post.jsx

+  const updateLike = (id, state) => {
+    console.log("업데이트 요청 API, id값은 ", id);
+    console.log("좋아요 요청", state);
+  };


좋아요 요청 함수인 updateLike가 모두 같은 기능을 한다면 여러 번 선언 하기 보다는 공통기능으로 분류 하는것이 더 좋을 것 같습니다.

넵!
현재 API가 나오지 않아서 임의로 만든 함수입니다.
수정하겠습니다!!

chimimode · 2023-10-10T15:41:12Z

src/components/Post.jsx

+  const updateLike = (id, state) => {
+    console.log("업데이트 요청 API, id값은 ", id);
+    console.log("좋아요 요청", state);
+  };


좋아요 기능이 짧은 시간에 연타로 여러번 반복 클릭 되는 경우도 존재 할 수 있을 것 같습니다. 이러한 상황을 예방 하기 위해 API요청 전 쓰로틀링이나 디바운싱을 고민 해 보는것도 좋을 것 같습니다.

오오..!!
저도 반복 클릭에 대한 고민이 있었습니다.
게시물의 좋아요 여부를 나타내는 bool값을 useState로 관리하여
좋아요가 false인 경우에만 api 요청을 하려고 했습니다.

이러한 방식은 제안 주신 방식에 비해 별로일까요??

말씀주신 방법도 좋은 해결책이 될 수 있을 것 같습니다. 다만 한번 좋아요 했던 게시글을 다시 취소 요청 하는 기능도 구현 할 경우 useState의 true/false만으로는 반복 클릭인지, 취소 요청인지 정보가 부족 할 수 있습니다. 이 부분도 같이 고려하거나 스펙아웃 시키면 괜찮을 것 같습니다.!

와 맞네요.. 생각하지 못했던 부분인 것 같습니다.
고려하여 개발하겠습니다.
감사합니다!!

chimimode · 2023-10-10T15:44:31Z

src/components/Layout.jsx

+  }
+  -ms-overflow-style: none; /* IE and Edge */
+  scrollbar-width: none; /* Firefox */
+`;


모바일 기기가 다양하다고 가정 했을 때 해상도에 따라 모든 메뉴가 보이지 않는 경우가 있는 것 같습니다.
(크롬 개발자 도구 기준 iPad Air, iPad Mini) 가로세로 비율에 따라 화면의 비율이 맞춰지도록 수정하는게 좋을 것 같습니다.!

ㅜㅜ 맞습니다..
저희끼리도 한번 이야기나눈 기억이 있습니다.
그때 내린 결론은 개발 기한이 한정되어있기 때문에
핵심 기능들을 어느 정도 구현 후 수정하기로 했습니다.

먼저 반응형으로 만드는 것이 좋을까요??

이미 다양한 케이스에 대해 고민하신 부분이 느껴지네요..! 메이저한 비율에 대해 먼저 우선순위를 두어 작업 한 뒤 엣지케이스에 해당하는 비율들은 추후에 작업되어도 괜찮을 것 같습니다. 👍

chimimode · 2023-10-10T15:55:31Z

src/pages/PopPage/PopPage.jsx

인기 페이지는 아래쪽으로 길어지는 스크롤이 예상되는 만큼 새로고침이나 위로가기 버튼과 같은 기능을 고려 해 보면 좋을 것 같습니다.

네!!! 좋은 기능일 것 같습니다.

chimimode · 2023-10-10T15:57:32Z

src/pages/PopPage/PopPage.jsx

아래쪽으로 스크롤이 길어질 때 마다 이미지가 입력 된 element가 누적되어 쌓이다 보면 한 페이지 내에 너무 많아져서 느려질 가능성이 있습니다. 이미 지나가서 보여지지 않아도 되는 element 에 대한 처리도 고민 해 보면 좋을 것 같습니다.!

와우!!! 좋은 말씀 감사합니다!!!!! 반드시 고민해보겠습니다!

chimimode · 2023-10-10T16:05:10Z

src/App.js

+      <Routes>
+        <Route path="/" element={<HomePage />}></Route>
+        <Route path="/pop" element={<PopPage />}>
+          <Route path="post/:postId" element={<PopPage />}></Route>


pop/post/:postId 페이지는 이미지를 보는 기능이 주된 기능이니, 만약 /pop/post/aaa 와 같이 postId값에 유효하지 않은 값이 오거나 올바르지 않은 요청으로 API 응답이 없었을 경우 처리에 대해 고민 해 봐도 좋을 것 같습니다.

넵!
아직 주소에 직접 접근하는 행위에 대한 처리는 아무것도 해두지 않은 상태입니다.
에러 처리를 다룰 때 더 신경쓰겠습니다!

chimimode · 2023-10-14T08:42:40Z

src/pages/SkeletonPage/SkeletonPage.jsx

(5주차)
로딩이 예상되는 컴포넌트들을 스켈레톤구성 해 둔 점이 유저 사용성에 신경쓰는 느낌이 있어 정말 좋습니다👍

chimimode · 2023-10-14T09:05:13Z

src/App.js

+        </Route>
+        <Route path="/profile" element={<MyPage />}></Route>
+        <Route path="/profile/setting" element={<SettingPage />}></Route>
+        <Route path="/test" element={<TestPage />}></Route>


(5주차)
테스트가 끝난 컴포넌트라면 route에서도 접근되지않도록 해당라인은 정리하는게 좋을 것 같습니다.!

넵!!
TestPage는 컴포넌트를 만들었을 때 어떻게 사용할지 예시를 커밋에 담아두는 용도로 사용하고 있었습니다.
아직까지는 추가로 사용할 수도 있어서.. 더 이상 사용되지 않을 것이라고 판단되면 삭제하겠습니다!!

chimimode · 2023-10-14T09:15:18Z

src/components/Post.jsx

+          <ModalButton
+            isLong
+            onClick={() => {
+              console.log("인스타그램 방문!");


(5주차)
이미 고려 하셨던 사항일 것 같습니다만 문의 겸 코멘트로 남깁니다.!
핵심기능으로 예상되는 인스타그램 방문 기능의 경우 유저가 브라우저에서 인스타그램관련 정보를 개발자도구 등으로 미리 알아내지 못하도록 프론트쪽에서는 정보가 드러나지않도록 구성 되어야 할 것 같습니다.

앗, 사실 이 부분에 대한 개발 경험이 없어서 아직 구체적인 생각을 해보지 않았습니다.. 😔
혹시 관련 레퍼런스나 검색 키워드를 추천해주실 수 있을까요??!!

예를들어 인스타그램 아이디를 프론트에서 props등으로 전달하거나 a링크로 직접 인스타 링크를 걸어 두는 경우 개발자도구에서 미리 열어 볼 수가 있기 때문에 폭죽을 사용하지 않고 인스타그램을 방문하는 우회방법이 있을수 있습니다.
따라서 프론트에서는 인스타그램에 대한 어떤 정보를 가지지 않는다고 가정하고, 해당 게시글의 Id나 업로드 작성자의 Id등의 정보만을 특정 API에 파라미터로서 넘기면 서버사이드에서 웹페이지를 이동시키는 처리를 하는 등의 방법으로 고려 해 볼 수 있을 것 같습니다.
해당 사항에 대해 백엔드쪽과 같이 고민 해 보는 시간이 있으면 좋을 것 같습니다. :D

iam454 and others added 30 commits September 24, 2023 15:11

docs: 리드미 수정

e949f6a

docs: 리드미 수정

9b9a9ce

Merge branch 'develop' into weekly

a1a76bd

Merge pull request #4 from Step3-kakao-tech-campus/weekly

4ab1d96

weekly -> develop 풀리퀘스트 테스트

docs: 리드미 수정

62e6d85

docs: 리드미 수정

bbf086f

chore: cra 프로젝트 생성

ab915f9

remove: 기본 파일 삭제

6201ea4

App.css index.css logo.svg 삭제

chore: styled-components 설치

c0ecb01

design: 스타일링 초기화

8c160de

reset css pretendard 글꼴 적용

design: theme 설정

c4e3cb8

color값 생성

chore: react-router-dom 설치

6eafe02

design: 레이아웃 생성

f4c169d

390*844 레이아웃 페이지 경로는 수정 예정

Merge branch 'feature/seo' into weekly

6d8161e

feat: 하단 내비게이션 바 생성

2d29dd5

test: 테스트 커밋

7d896d0

Merge pull request #5 from iam454/weekly

b99a98f

fork해서 pr 보내는 테스트 -> merge까지 진행

test

dc10a5b

Merge pull request #6 from MINJOO-KIM/weekly

ae1651d

fork해서 PR하기

feat: 현재 위치에 따라 내비게이션 색상 변경 기능 생성

42aaaf6

Merge pull request #7 from iam454/weekly

3349d33

현재 위치에 따라 내비게이션 색상 변경 기능 생성

feat: 인기페이지 컨테이너 생성 및 내비게이션바 위치 수정

232945f

chore: localtunnel 설치

8eebb8a

테스트용

feat: 모바일 화면에 대응할 수 있도록 변경

cb352de

feat: 인기 게시물 리스트용 Card 컴포넌트 생성

42ab9b9

가짜 데이터 사용

chore: egjs/react-infinitegrid 설치

553835a

feat: masonry infinite grid 적용

bbc6e3d

가짜 데이터 사용 실제 데이터 페칭 때 더 봐야할 듯

Merge pull request #8 from iam454/weekly

685c2a2

모바일 환경에 대응할 수 있도록 변경, 인기 페이지 레이아웃 임시 생성, 로컬 테스트용 라이브러리 localtunnel 설치

feat: 모달용 버튼 생성

4240e37

rename: 아이콘 관련 이미지 위치 변경

af0f89e

iam454 and others added 18 commits October 4, 2023 17:42

feat: 좋아요 UI 생성(미완성)

2841ea4

중앙 좋아요 애니메이션 구현은 안 된 상태 Post 중복된 코드 줄이는 법 생각해야함

Merge branch 'feature/1003' into weekly

2675cad

chore: swiper 설치

Merge pull request #18 from iam454/weekly

1c6138a

좋아요 애니메이션 생성 및 Post.jsx 수정

Merge pull request #19 from MINJOO-KIM/weekly

57184b1

Swiper 설치

feat: 설정 버튼 추가

243e92c

Merge branch 'feature/park' into weekly

5ddb6b3

feat: 마이페이지 틀 제작

62d60e9

feat: 더블 클릭시 중앙에 좋아요 애니메이션 생성

b1aba98

Merge pull request #21 from iam454/weekly

d469e2a

더블 클릭시 중앙에 좋아요 애니메이션 생성

feat: 설정 페이지 UI 생성

aef4aa1

Merge pull request #20 from GhoRid/weekly

9e076f9

마이페이지 틀 제작

Merge branch 'weekly' into weekly

82c18c2

Merge pull request #22 from iam454/weekly

f092a5f

설정 페이지 UI 생성 + 컨플릭트 해결

feat: 홈 페이지 swiper 적용

3453fb5

pause/resume 가능 time progress bar 현재는 5초로 지정 클릭/더블클릭 이벤트 구분해야함 react-modal overlay click issue

Merge branch 'feature/swiper' into weekly

556abaa

Merge pull request #23 from iam454/weekly

8ad4fe1

홈 페이지 Swiper 적용

docs: 리드미 수정

952de73

Merge branch 'weekly' into develop

66ed464

iam454 merged commit 070613c into review Oct 8, 2023

chimimode reviewed Oct 10, 2023

View reviewed changes

chimimode reviewed Oct 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

전남대_14조_코드리뷰_4주차 #14

전남대_14조_코드리뷰_4주차 #14

iam454 commented Oct 1, 2023

iam454 commented Oct 8, 2023

chimimode Oct 10, 2023

iam454 Oct 10, 2023

chimimode Oct 10, 2023

iam454 Oct 10, 2023

chimimode Oct 10, 2023

iam454 Oct 11, 2023

chimimode Oct 11, 2023

iam454 Oct 12, 2023

chimimode Oct 10, 2023

iam454 Oct 11, 2023

chimimode Oct 11, 2023

chimimode Oct 10, 2023

iam454 Oct 11, 2023

chimimode Oct 10, 2023

iam454 Oct 11, 2023

chimimode Oct 10, 2023

iam454 Oct 11, 2023

chimimode Oct 14, 2023

chimimode Oct 14, 2023

iam454 Oct 14, 2023

chimimode Oct 14, 2023

iam454 Oct 14, 2023

chimimode Oct 14, 2023

전남대_14조_코드리뷰_4주차 #14

전남대_14조_코드리뷰_4주차 #14

Conversation

iam454 commented Oct 1, 2023

✅ 산출물

❓ 질문

❗️ 추가로

iam454 commented Oct 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment