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

[김한샘] Week12 #390

Conversation

hansaemkim38
Copy link
Collaborator

요구사항

기본

  • TypeScript를 활용해 프로젝트의 필요한 곳에 타입을 명시해 주세요.
  • ‘/shared’, ‘/folder’ 페이지에 현재 폴더에서 검색어가 포함된 링크를 찾을 수 있는 링크 검색 기능을 만들어 주세요.

주요 변경사항

  • 시간이 부족하여 기존 코드에 타입만 명시 했습니다.

멘토에게

  • 늦어서 죄송합니다.....
  • 타입 명시 위주로 리뷰해주시면 감사하겠습니다.
  • 멘토님 감사합니다. :)

@hansaemkim38 hansaemkim38 requested a review from Jay-WKJun May 6, 2024 09:21
@hansaemkim38 hansaemkim38 reopened this May 6, 2024
@hansaemkim38 hansaemkim38 added the 미완성🫠 죄송합니다.. label May 6, 2024
Copy link
Collaborator

@Jay-WKJun Jay-WKJun left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 👍

정말 죄송합니다. ㅠㅠㅠ 이번주까지 제가 담당인지 몰랏네요. 😭

  • 필요할때마다 선언해둔 느낌이네요. 하나의 큰 타입을 두고 재활용하는 것이 좋습니다. (DRY 원칙)
  • 변수명이 일관적이지 않고 보편적인 개발 컨벤션과 조금 다릅니다. 다른 분들의 코드를 더 많이 봐보세요.
  • React에서 제공하는 타입들을 잘 활용해주셨네요!

Utility Type에 대해 알아보면 좋을 것 같습니다.

수고하셨습니다! 😁

@@ -9,14 +10,27 @@ import EditableStarButton from "./EditableStarButton";
import KebabButton from "./KebabButton";
import { useLocation } from "react-router-dom";

function Card({ card }) {
interface FolderCardData {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Card 컴포넌트의 Prop에 대한 type인데 CardPropType 정도로 표현하면 더 직관적이지 않을까요?


function KebabList({ url, setDisplay }) {
const { openModal, setModalType, setCardUrl } = useContext(ModalContext);
interface KebabListInterface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

변수명에 변수 타입이 들어가는 것이 안좋듯이, type과 interface도 마찬가지입니다.

Comment on lines +14 to +25
interface UserFolderCardDataList {
data: {
id: number;
createdAt: string;
description?: string;
folderId?: number;
title?: string;
updatedAt?: string;
url: string;
imageSource?: string;
};
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

바로 위에 선언한 CardListData를 재활용하는 것이 좋습니다.

TS의 & 를 잘 이용해보세요.

import React from "react";
import "./Button.css";

interface buttonData {
Copy link
Collaborator

Choose a reason for hiding this comment

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

타입의 이름들은 파스칼케이스로 작성해주시는 편이 좋습니다.

function FolderAddModal({ modalTypeLabels, modalType, cardUrl, folderTabDataList }) {
const [activeId, setActiveId] = useState(null);
interface UserFolderdataList {
data: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

굳이 data라는 depth가 필요할까요?

이 또한, 필요할 때마다 다른 내용을 보면서 선언하거나 하나의 정리된 타입을 사용하지 않아서 발생하는 이상한 타입의 전형입니다.

타입을 더 큰 범위에서 정리해보는 것이 좋을 것 같습니다.

Comment on lines +8 to +25
interface FolderData {
id: number;
name: string;
owner: {
id: number;
name: string;
profileImageSource: string;
};
links: {
id: number;
createdAt: string;
url: string;
title: string;
description: string;
imageSource?: string;
}[];
count: number;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

몇번이나 봤던 interface가 계속 나오는거 같네요.

하나의 type으로 관리해보는 것이 좋을 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

감사합니다.!

@Jay-WKJun Jay-WKJun merged commit 491b5cd into codeit-bootcamp-frontend:part2-김한샘 May 10, 2024
1 check passed
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