Skip to content

[이진우] Week13 #1024

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 3 commits into
base: part2-이진우
Choose a base branch
from

Conversation

yeeZinu
Copy link
Collaborator

@yeeZinu yeeZinu 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으로 만들었나요?

  • 링크를 입력하고 “추가하기” 버튼을 누르면 “폴더에 추가” 모달이 보이나요?

  • “폴더 추가” 버튼을 누르면 “폴더 추가” 모달이 보이나요?

  • “공유” 버튼을 누르면 “폴더 공유” 모달이 보이나요?

  • “폴더 추가” 버튼을 누르면 “폴더 추가” 모달이 보이나요?

  • “삭제” 버튼을 누르면 “폴더 삭제” 모달이 보이나요?

  • 케밥 버튼을 누르면 “삭제하기”, “폴더에 추가” 버튼이 있는 팝오버가 보이나요?

  • 팝오버에 있는 “삭제하기” 버튼을 누르면 “링크 삭제” 모달이 보이나요?

  • 팝오버에 있는 “폴더에 추가” 버튼을 누르면 “폴더에 추가” 모달이 보이나요?

  • “폴더 공유” 모달에서 “카카오톡 아이콘”을 클릭하면 카카오로 공유 폴더 페이지 링크 공유하기 가능한가요?

  • “폴더 공유” 모달에서 “페이스북 아이콘”을 클릭하면 페이스북으로 공유 폴더 페이지 링크 공유하기 가능한가요?

  • “폴더 공유” 모달에서 “링크 아이콘”을 클릭하면 클립보드에 공유 폴더 페이지 링크가 복사 되나요?

주요 변경사항

  • 8~9 주차 내용 구현했습니다
  • React + TS 로 마이그레이션 했습니다

스크린샷

image

멘토에게

  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

Copy link

vercel bot commented Apr 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
4-weekly-mission ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 8, 2024 11:28am

@yeeZinu yeeZinu requested a review from greenblues1190 April 8, 2024 11:28
@yeeZinu yeeZinu self-assigned this Apr 8, 2024
@yeeZinu yeeZinu added the 순한맛🐑  마음이 많이 여립니다.. label Apr 8, 2024
@yeeZinu yeeZinu changed the title [Week13] 이진우 [이진우] Week13 Apr 8, 2024
Copy link
Collaborator

@greenblues1190 greenblues1190 left a comment

Choose a reason for hiding this comment

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

코드 분량이 많아 나눠서 리뷰하겠습니다~

RCA

각 리뷰 앞에 r, c, a로 리뷰의 우선순위를 나타냈으니 참고 부탁드립니다.

  • r: Request Change. 꼭 반영해주세요.
  • c: Comment. 참고해주세요.
  • a: Approve. 바로 머지 가능한 코드입니다.

import LinkImage, { ImageCard } from './LinkImage';
import LinkInfo, { InfoGroup } from './LinkInfo';

const LinkItem = styled.li`
Copy link
Collaborator

Choose a reason for hiding this comment

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

c: 링크카드는 항상 li(리스트 아이템) 태그인가요? 👀

const [showPopover, setShowPopover] = useState(false);

const handleKebabClick = (e: MouseEvent) => {
e.preventDefault();
Copy link
Collaborator

Choose a reason for hiding this comment

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

c: 버튼 클릭시 preventDefault를 호출하신 이유가 궁금해요

Comment on lines +5 to +8
interface DeleteFolderProps {
selectedFolderName: string;
onCloseModal: () => void;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

r: *Props로 컴포넌트 프롭타입을 분리한 코드와 안한 코드가 섞여있네요 👀 일관성 유지 측면에서 더 좋다고 생각하시는 방법으로 통일해주세요

Comment on lines +30 to +31
if (!(window as any).Kakao.isInitialized()) {
(window as any).Kakao.init('92da1d3b4c5ad0b1169a7a32913482aa');
Copy link
Collaborator

Choose a reason for hiding this comment

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

c: Kakao SDK가 타입스크립트를 지원안해 window 객체를 any로 as 단언하셨군요. globals.d.ts 파일에 { Kakao: unknown } 정도만 정의해두면 어떨까 생각이 듭니다!

type="text"
placeholder="내용 입력"
value={value}
className={`${styles.input} ${active && styles.active}`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

c: 다음처럼 Array 메서드를 사용해 클래스네임 여러개를 이어붙일 수도 있어요. classnames 라이브러리가 이렇게 구현되었습니다.

Suggested change
className={`${styles.input} ${active && styles.active}`}
className={
[styles.input, active && styles.active]
// 값이 있는 요소만 필터링 (active가 false면 styles.active가 필터링에서 제외됨)
.filter(Boolean)
// 공백으로 붙이기
.join(' ')
}

const [user, setUser] = useState<User>();

useEffect(() => {
const handleUserLoad = async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

c: handle~이라는 접두사는 어떤 기준으로 붙이고 계신가요?

Comment on lines +13 to +15
<a href="/" className={styles.logoArea}>
<img src={linkbraryLogo} alt="로고" className={styles.logoImg} />
</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

r: 리액트에서 a 태그를 사용하면 페이지가 새로고침되어 리액트 상태가 초기화되기 때문에 외부 링크로 이동하지 않는 이상 react-router의 Link 컴포넌트나 navigator를 사용해 추후 개선해주세요~

Comment on lines +27 to +29
<a className={styles.btn} href="/signin.html">
로그인
</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

r: 아직 구현이 되지 않은 부분이네요 이부분도 추후 개선해주세요

Comment on lines +11 to +23
const handleFolderLoad = async () => {
try {
const {
folder: { name, owner },
} = await getSampleFolder();

setFolderName(name);
setUserName(owner.name);
setProfileImage(owner.profileImageSource);
} catch (err) {
console.error(err);
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

r: 계속 해서 반복되는 useEffect, API 요청, setState, 에러 catch에서 패턴이 보이신다면 훅으로 분리, 재사용해 개선해주세요

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
순한맛🐑  마음이 많이 여립니다..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants