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

[우제윤] week13 #402

Merged

Conversation

wjy308
Copy link
Collaborator

@wjy308 wjy308 commented May 10, 2024

요구사항

기본

  • React 프로젝트에서 진행했던 작업물을 Next.js 프로젝트에 맞게 변경 및 이전해 주세요.
  • [] 팀내 2 ~ 3명이 짝을 지어 페어프로그래밍으로 로그인, 회원가입 페이지에 사용하는 Input 컴포넌트를 만들어 주세요.

주요 변경사항

  • 기존 React 프로젝트를 Next.js로 변경하는 작업까지 완료했습니다.

스크린샷

멘토에게

  • 아직 미완성 부분들이 남아있습니다..!
  • 따로 기재하여 추후 꼭 완성시키도록 하겠습니다!
  • 지난 리뷰에서 타입스크립트 명시 관련 리뷰를 받지 못하여 이 부분 이번주에 부탁 드리겠습니다!
  • FolderCardListItem.tsx 파일의 a태그에 이벤트 버블링 현상이 일어나는것 같습니다! (케밥 버튼 클릭 시 여전히 a태그의 이벤트가 발생) 해당 부분 해결하기가 조금 어렵습니다!
    folderCardListItem
    케밥 이벤트
    이런식으로 모달창을 띄워도 모달 창 바깥 부분에서 a태그의 hover이벤트 라던지 클릭 이벤트가 발생합니다!

미완성 요구사항

  • 11주차)

    • [] “폴더 공유” 모달의 기능은 클릭시 ‘{호스트 주소}/shared/{currrentFolderId}’ 로 접속 가능한 주소를 공유 또는 클립보드에 복사 가능하게 해주세요.
    • [] 팀내 2 ~ 3명이 짝을 지어 페어프로그래밍으로 여러 모달을 만들 때 사용할 모달 컴포넌트를 만들어 주세요.
  • 12주차)

    • [] ‘/shared’, ‘/folder’ 페이지에 현재 폴더에서 검색어가 포함된 링크를 찾을 수 있는 링크 검색 기능을 만들어 주세요.
  • 13주차)

    • [] 팀내 2 ~ 3명이 짝을 지어 페어프로그래밍으로 로그인, 회원가입 페이지에 사용하는 Input 컴포넌트를 만들어 주세요.

@wjy308 wjy308 added the 미완성🫠 죄송합니다.. label May 10, 2024
@wjy308 wjy308 assigned wjy308 and unassigned kich555 May 10, 2024
@wjy308 wjy308 requested a review from kich555 May 10, 2024 13:39
@wjy308 wjy308 changed the base branch from main to part3-우제윤 May 10, 2024 16:29
@kich555
Copy link
Collaborator

kich555 commented May 13, 2024

안녕하세요 제윤님! 몇가지 궁금한 사항이 있는데요 😅

  1. 기존 프로젝트를 Next.js로 마이그레이션하는 과제같은에 왜 모든 파일들이 새로 만들어진 파일인건지가 궁금합니다
  2. 스타일드 컴포넌트와 module.css를 같이 사용하시는 이유가 혹시 있으실까요? 스타일 관리 주체가 2개가 되어야하는 특별한 이유가 있나 싶어서요

@kich555
Copy link
Collaborator

kich555 commented May 13, 2024

�css 속성들을 나열할때도 일종의 컨벤션이 필요합니다!

저는 개인적으로 display > position > box-model > typography 와 관련된 속성 위주로 정렬하는 편이에요!! 만약 module.css를 계속 사용하실 예정이라면 styleLint와 같은 linter를 사용해보는것도 좋을것 같습니다 ㅎ

@kich555
Copy link
Collaborator

kich555 commented May 13, 2024

캐밥 컴포넌트의 버튼 등에서 event.stopPropagation()을 사용해서 말씀하신 FolderCardListItem 까지의 이벤트 버블링을 막을 수 있어요!

Copy link
Collaborator

@kich555 kich555 left a comment

Choose a reason for hiding this comment

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

굳굳 고생하셨습니다!👍👍👍
볼륨이 커서 특히 더 고생하셨을것 같네요 ㅜ
잘 해주고 계신것 같아요!!
고생하셨습니다!


.CardList {
display: grid;
grid-template-columns: repeat(32.5rem);
Copy link
Collaborator

Choose a reason for hiding this comment

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

이거 이렇게 작성하면 동작하나요?
grid-template-columns: repeat(auto, 32.5rem) 이렇게 수정되어야할 것 같은데..한번 확인해주시겠어요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

둘 다 정상적으로 작동하는것 같습니다! 일단 멘토님이 작성해준대로 수정 진행하겠습니다!
혹시 두 코드 차이가 어떻게 되는지 알 수 있을까요?

margin-top: 20px;
justify-content: center;
width: 100%;
max-width: 106rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

수치 단위도 하나로 통일하는게 좋아요!
px을 사용했으면 계속 px을, rem을 사용했으면 계속 rem을 사용하는 방식으로 수정하면 더 좋겠네요 ㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

px를 사용했는데 현업에서는 rem 위주로 사용한다 들어서 후에 변경하다보니 뒤죽박죽 되어있네요..! 최대한 rem으로 통일하겠습니다!

.CardList_item {
display: flex;
flex-direction: column;
width: 325px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

디자이너분들도 우리 개발자처럼 보통의 컨벤션을 가지고있는데요,
대표적으로 수치들이 모두 2의 배수 혹은 4의 배수로 떨어지게끔 디자인 하는것입니다 ㅎ
만약 다른부분에서는 4의배수, 2의배수 같은 컨벤션이 있지만 특정 부분에서만 해당 컨벤션이 깨진다면
저라면 디자이너분께 넌지시 한번 여쭤볼것 같아요 ㅎ

position: relative;
}

.CardList-item-createdAt {
Copy link
Collaborator

Choose a reason for hiding this comment

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

이렇게 사용하지 않는 클래스는 삭제해주세요!
만약 삭제하면 안될 이유가 있다면 주석을 남겨주시는게 좋습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

useAsync라는 이름은 너무 추상적인 것 같습니다. 만약 해당 이름을 가지려면 정말
모든 비동기를 처리하는 훅이어야 해요
하지만 내용을 보니 이 훅은 유저 정보와 폴더를 받아와서 활용하는 훅 같은데요
이름을 useUserContext 등으로 바꿔보시는건 어떠세요? ㅎ

@@ -0,0 +1,75 @@
import "@/components/CardList/CardList.module.css";
import { useState, useEffect } from "react";
Copy link
Collaborator

Choose a reason for hiding this comment

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

import { useState, useEffect } from "react";와
import React from "react"; 는 한줄로 처리되어야할텐데...
혹시 기본적인 eslint세팅을 안하고 작업을 진행하신다면
eslint와 prettier는 꼭 항상 세팅하고 작업해주세요!!!

const BASE_URL = "https://bootcamp-api.codeit.kr/api";

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

Choose a reason for hiding this comment

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

이거 아래쪽에 api함수로 별도의 파일에서 관리하는것 같은데..그걸 사용하는쪽으로 수정하는게 더 좋아보이네요!

const [curFolder, setCurFolder] = useState(null);
const [folderId, setFolderId] = useState<number>(0);

const handleFolderClick = (folder: any) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

any타입을 쓸수밖에없는 상황이라면 차라리 unkonwn 타입을 사용하는게 더 좋습니다.


return (
<>
{folderList && folderList.data && folderList.data.length > 0 ? (
Copy link
Collaborator

Choose a reason for hiding this comment

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

folderList.data.length > 0 이런건 lodash를 사용해서
isEmpty(folderList.data.length) 와 같이 사용하는게 가독성 측면에서 더 좋을것 같아요 !
lodash를 적극 활용해주세요!

@@ -0,0 +1,78 @@
import { useState } from "react";
import * as S from "./FolderCardListItemStyled";
Copy link
Collaborator

Choose a reason for hiding this comment

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

S......는 다른거로 바꿔주시는게 좋을것 같아요
다른사람이 본다면 S가 무엇을 의미하는지 한눈에 알 수 없을것 같아요 ㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

혹시 추천하는 표기법이나 현업에서 자주 사용하는 표기법이 있을까요?

@kich555 kich555 merged commit de4d814 into codeit-bootcamp-frontend:part3-우제윤 May 13, 2024
@wjy308
Copy link
Collaborator Author

wjy308 commented May 13, 2024

안녕하세요 제윤님! 몇가지 궁금한 사항이 있는데요 😅

  1. 기존 프로젝트를 Next.js로 마이그레이션하는 과제같은에 왜 모든 파일들이 새로 만들어진 파일인건지가 궁금합니다
  2. 스타일드 컴포넌트와 module.css를 같이 사용하시는 이유가 혹시 있으실까요? 스타일 관리 주체가 2개가 되어야하는 특별한 이유가 있나 싶어서요
    --> 강의를 들으면서 새로운 방법이 나와서 다양한 방법으로 연습해보는 과정에서 섞인것 같습니다!

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