Skip to content
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

[Week8] 박상준 #344

Merged
merged 1 commit into from
Apr 10, 2024
Merged

Conversation

sj0724
Copy link
Collaborator

@sj0724 sj0724 commented Apr 8, 2024

요구사항

기본

  • 링크 공유 페이지 url path는 ‘/shared’, 폴더 페이지 url path는 ‘/folder’가 되도록 설정했나요?

  • 폴더 페이지에서 상단 네비게이션 바는 스크롤 시 상단에 고정하지 않고 가려지도록 했나요?

  • 상단 네비게이션 바에 프로필 영역의 데이터는 https://bootcamp-api.codeit.kr/docs 에 명세된 “/api/users/1”을 활용했나요?

  • “전체” 폴더를 선택한 경우 “공유”, “이름 변경”, “삭제” 버튼들이 보이지 않지만, 다른 폴더를 선택한 경우에는 버튼들이 보이나요?

  • 폴더 목록에 필요한 데이터는 “/api/users/1/folders”를 활용했나요?

  • “전체” 폴더에 필요한 링크들 데이터는 “/api/users/1/links”를 활용하고, 이외의 폴더에 필요한 링크들 데이터는 “/api/users/1/links?folderId={해당 폴더 ID}”를 활용했나요?

  • Mobile에서 “폴더 추가” 버튼은 최하단에서 101px 떨어져있는 Floating Action Button으로 만들었나요?

심화

  • [x]
  • []

주요 변경사항

스크린샷

image

멘토에게

  • styled component를 별도의 파일로 분리에서 import 해서 사용했는데 이런 방식으로 사용을 많이 하나요?

@sj0724 sj0724 requested a review from kiJu2 April 8, 2024 12:06
@sj0724 sj0724 self-assigned this Apr 8, 2024
@sj0724 sj0724 added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label Apr 8, 2024
@kiJu2
Copy link
Collaborator

kiJu2 commented Apr 10, 2024

수고 하셨습니다 ! 위클리 미션 하시느라 정말 수고 많으셨어요.
학습에 도움 되실 수 있게 꼼꼼히 리뷰 하도록 해보겠습니다.

@kiJu2
Copy link
Collaborator

kiJu2 commented Apr 10, 2024

styled component를 별도의 파일로 분리에서 import 해서 사용했는데 이런 방식으로 사용을 많이 하나요?

넵 ! 코드가 길어지면 파일을 분리해서 관리하기도 합니다 !
만약 styled component로 작성한 코드가 짧다면 그냥 한 파일에 넣는게 직관적이고 생산성이 좋아질거라고 생각해요.
이러한 건 특별한 컨벤션이 없을 경우 유연하게 작성하시는걸 권장드립니다 ㅎㅎㅎ 😊

Comment on lines +14 to +15
"styled-component": "^2.8.0",
"styled-components": "^6.1.8",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

패키지가 잘못된 것 같아요 !

Suggested change
"styled-component": "^2.8.0",
"styled-components": "^6.1.8",
"styled-components": "^6.1.8",

styled-component는 흔히 사용하는 라이브러리가 아닙니다 😊

터미널에서 npm uninstall styled-component 하시고 commit한 번 해주시면 됩니다 ~!

@@ -0,0 +1,27 @@
import axios from "../instance/axiosInstance";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

axios가 아닌 instance라는 명칭이 어떨까요?

Suggested change
import axios from "../instance/axiosInstance";
import instance from "../instance/axiosInstance";

사실, 큰 건 아니지만 추 후 새로운 인스턴스를 생성해야 할 때 진짜 axios를 사용할 수도 있는데
라이브러리를 사용하는 건지, 내부에서 정의한 인스턴스를 사용하는건지 헷갈릴 수도 있을 것 같아서요. 🫠

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 파일은 constants/ 을 만드시고 그 안에 넣는게 더 좋을 것 같아요 ! 😊

const [sectionList, setSectionList] = useState([]);

useEffect(() => {
setSectionList(sectionDescription);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기서 사용하고 계시군요.

이럴 때는 코드 자체가 짧기 �때문에 해당 파일 내에 작성하시는 것도 고려해봐도 좋습니다 !

파일 내부에서 선언하고 사용할 경우

직관적이며 파일을 분리하지 않아도 되므로 생산성이 증가해요.

main.constant.js를 만들어서 사용할 경우

관심사가 분리 되기에 가독성이 증가한다는 장점이 있습니다 !


각각 한 번 저울질 해보시고 적용해보세요 😊

Comment on lines +16 to +35
useEffect(() => {
const nowDate = new Date();
let createdate;
if (item.createdAt) {
createdate = new Date(item.createdAt);
} else if (item.created_at) {
createdate = new Date(item.created_at);
}
const date = (nowDate - createdate) / 1000;
setCreatedAt(calculateDate(date));
setFullDate(changeDate(createdate));
}, [item]);

useEffect(() => {
if (item.imageSource) {
setImageUrl(item.imageSource);
} else if (item.image_source) {
setImageUrl(item.image_source);
}
}, [item]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

item에 대한 deps가 두 번 잡혀있어요.

하나로 통합하는게 어떨까요?

Suggested change
useEffect(() => {
const nowDate = new Date();
let createdate;
if (item.createdAt) {
createdate = new Date(item.createdAt);
} else if (item.created_at) {
createdate = new Date(item.created_at);
}
const date = (nowDate - createdate) / 1000;
setCreatedAt(calculateDate(date));
setFullDate(changeDate(createdate));
}, [item]);
useEffect(() => {
if (item.imageSource) {
setImageUrl(item.imageSource);
} else if (item.image_source) {
setImageUrl(item.image_source);
}
}, [item]);
useEffect(() => {
// 생성 날짜 처리
const createdate = new Date(item.createdAt || item.created_at);
const dateDiffInSeconds = (new Date() - createdate) / 1000;
setCreatedAt(calculateDate(dateDiffInSeconds));
setFullDate(changeDate(createdate));
// 이미지 URL 처리
setImageUrl(item.imageSource || item.image_source);
}, [item]); // 의존성 배열에 item만 포함

Comment on lines +30 to +34
if (item.imageSource) {
setImageUrl(item.imageSource);
} else if (item.image_source) {
setImageUrl(item.image_source);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분은 불필요한 로직으로 보여요.

imageSourceimage_source 두 개를 분기하고 있습니다. 서버와 �통신하는 함수에서 imageSource 하나로만 반환될 수 있도록 하면 어떨까요?
그렇게 하지 않으면 모든 곳에서 위와 같이 검사를 해야할 것으로 보여요.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이는 앞으로 개발을 하면서 더욱 늘어나기만 할 것이기 때문에 조기에 빠르게 리팩토링 하시는걸 권장드립니다 👍

Comment on lines +10 to +17
const loadUser = async () => {
const reuslt = await getSampleUser();
setProfile(reuslt);
};

useEffect(() => {
loadUser();
}, []);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadUseruseEffect 내부에 선언하는게 어떨까요?

Suggested change
const loadUser = async () => {
const reuslt = await getSampleUser();
setProfile(reuslt);
};
useEffect(() => {
loadUser();
}, []);
useEffect(() => {
const loadUser = async () => {
const reuslt = await getSampleUser();
setProfile(reuslt);
};
loadUser();
}, []);

Application이 리렌더링 될 때 마다 불필요한 재선언을 하게됩니다. useEffect내부에 선언하는게 어떨까요?

import { useEffect, useState } from "react";
import { getSampleUser } from "../api/api";

function Application() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

파일과 함수 네이밍이 다른 것 같아요 😊

Suggested change
function Application() {
function Layout() {

Comment on lines +12 to +29
useEffect(() => {
switch (item.id) {
case 1:
setSectionImage(Image1);
break;
case 2:
setSectionImage(Image2);
break;
case 3:
setSectionImage(Image3);
break;
case 4:
setSectionImage(Image4);
break;
default:
break;
}
}, [item.id]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

itempath가 있도록 하면 어떨까요?

최종적으로 sectionDescription에 image에 따른 url이 있으면 어떨까 싶어요

@kiJu2
Copy link
Collaborator

kiJu2 commented Apr 10, 2024

수고하셨습니다 상준님 !
이제 점차 리액트에 적응되어가시는게 눈에 보이는군요 😊💪

@kiJu2 kiJu2 merged commit c61d9ea into codeit-bootcamp-frontend:part2-박상준 Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants