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

[임채민] Week20 #1085

Open
wants to merge 46 commits into
base: part3-임채민
Choose a base branch
from

Conversation

Chaemin-153
Copy link
Collaborator

주요 변경사항

  • 파트3 프로젝트 이후 전체적으로는 앱라우터로 변경했고, 15,16주차 요구사항 구현하고자 했습니다
  • 아직 미완된 부분이 많고, 20주차 요구사항은 적용하지 못했습니다 ㅜ

스크린샷

image

멘토에게

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

withyj-codeit and others added 30 commits November 6, 2023 16:43
Folder 페이지에서, FolderHeader 파일에 '링크 추가 input' 생성
Copy link

vercel bot commented May 13, 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 May 13, 2024 8:03pm
4-weekly-mission-ursd ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 13, 2024 8:03pm

@kiJu2
Copy link
Collaborator

kiJu2 commented May 15, 2024

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

@kiJu2
Copy link
Collaborator

kiJu2 commented May 15, 2024

파트3 프로젝트 이후 전체적으로는 앱라우터로 변경했고, 15,16주차 요구사항 구현하고자 했습니다

굳굳 좋습니다 ! 😊

아직 미완된 부분이 많고, 20주차 요구사항은 적용하지 못했습니다 ㅜ

넵 괜찮습니다 ㅎㅎ 변경된 사항 위주로 꼼꼼히 살펴볼게요 !

Comment on lines +5 to +19
domains: [
'img1.daumcdn.net',
'yt3.googleusercontent.com',
'velog.velcdn.com',
'avatars.githubusercontent.com',
'codeit-images.codeit.com',
'codeit-frontend.codeit.com',
'reactjs.org',
'assets.vercel.com',
'storybook.js.org',
'testing-library.com',
'static.cdninstagram.com',
's.pstatic.net',
'tanstack.com',
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

아마, 앱라우터 정책 상 외부 이미지를 화이트리스트로 받는다는 점에서 이렇게 설정하신 것 같군요.

요구사항은 사용자가 이미지 URL을 넣도록 되어 있어서 '모든 URL'이 될 수 있다는 점과 NextJS 보안 정책에 의해 막히고 있는 듯 합니다.
현재 채민님께서 대부분의 도메인들을 허용해주고 있으나 근본적인 해결책은 아닌 것을 자각하고 계실거라 생각해요.

이럴 경우 모든 도메인을 허용 **해주는 방법이 있으나 이는 NextJS 외부 이미지를 통한 보안을 해제하는 것과 다름 없습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

그럼 어떻게 할까?

제가 제안드리는 솔루션은 특정 컴포넌트에서만 모든 외부 이미지를 허용해주는게 어떨지 제안드립니다:

function AllowedExternalImage({
  src,
  width,
  height,
  alt,
}: Pick<ImageProps, "src" | "width" | "height" | "alt">) {
  return (
    <Image
      loader={({ src }) => src}
      src={src}
      alt={alt}
      width={width}
      height={height}
      unoptimized={true}
    />
  );
}

export default function Card({ title, image, id }: PostSchema) {
  return (
    <Link href={`/posts/${id}`} scroll={false}>
      <div className="flex flex-col justify-center items-center p-4 border border-gray-300 rounded-lg cursor-pointer hover:shadow-lg transition-shadow">
        {/* <Image
          src={image}
          alt={title}
          width={300}
          height={300}
          className="rounded-lg"
        /> */}
        <AllowedExternalImage src={image} width={300} height={300} alt={title} />
        <h2 className="text-2xl font-bold">{title}</h2>
      </div>
    </Link>
  );
}

위처럼 설정해두면 Card 컴포넌트에서만 모든 외부 도메인이 허용될 수 있습니다. 😊

@@ -0,0 +1,19 @@
'use client';
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 +12
<Head>
<title>Linkbrary - Folder</title>
</Head>
Copy link
Collaborator

Choose a reason for hiding this comment

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

앱 라우터에서는 다음과 같이 title을 작성할 수 있습니다:

import type { Metadata } from 'next'
 
// either Static metadata
export const metadata: Metadata = {
  title: '...',
}
 
// or Dynamic metadata
export async function generateMetadata({ params }) {
  return {
    title: '...',
  }
}

자세히 보기

Comment on lines +11 to +17
useEffect(() => {
const accessToken = localStorage.getItem('accessToken');

if (!accessToken) {
router.push('/signin');
}
}, []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 값을 AuthContext로 따로 만드는건 어떨까요?

컨텍스트와 컨텍스트 훅을 사용해볼 수 있을 것 같습니다.
덤으로 루트 layout을 서버 컴포넌트로 지정할 수 있겠네요. 😊

@@ -0,0 +1,7 @@
'use client';
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
'use client';

Comment on lines +85 to +87
<Head>
<title>Linkbrary - Signin</title>
</Head>
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 코드 또한 Metadata로 작성될 수 있습니다:

import type { Metadata } from 'next'
 
// either Static metadata
export const metadata: Metadata = {
  title: '...',
}
 
// or Dynamic metadata
export async function generateMetadata({ params }) {
  return {
    title: '...',
  }
}

자세히 보기

<button
className={styles.submitBtn}
type="submit"
disabled={!isBtnActive}
Copy link
Collaborator

Choose a reason for hiding this comment

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

disabled는 상태 관리로 관리할 필요가 없습니다 😊:

Suggested change
disabled={!isBtnActive}
disabled={!(email.trim() !== '' &&
password.trim() !== '' &&
password.trim().length >= 8 &&
password === confirmPassword &&
//... 그 외 validations...
)}

위 코드처럼 표현할 수 있어요. 다만 조건부 렌더링이 복잡해지겠죠?
다음과 같이 해볼 수도 있어요:

 const isValidSignupForm = (email.trim() !== '' &&
        password.trim() !== '' &&
        password.trim().length >= 8 &&
        password === confirmPassword &&
        //... 그 외 validations...
      );

// ...
// 렌더링 부분에서 ..
          <button
            className={styles.submitBtn}
            type="submit"
            disabled={!isBtnActive}

Copy link
Collaborator

Choose a reason for hiding this comment

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

그리고 useMemo를 활용하여 최적화를 시킬 수도 있습니다 !

import { useMemo } from 'react';

function TodoList({ todos, tab }) {
  const visibleTodos = useMemo(
    () => filterTodos(todos, tab),
    [todos, tab]
  );
  // ...
}

useMemo

Comment on lines +23 to +27
if (!editFolderModalIsOpen) {
setEditFolderModalIsOpen(true);
} else {
setEditFolderModalIsOpen(false);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

다음과 Not gate만으로 간단하게 토글 함수를 만들 수 있습니다 😊:

Suggested change
if (!editFolderModalIsOpen) {
setEditFolderModalIsOpen(true);
} else {
setEditFolderModalIsOpen(false);
}
setEditFolderModalIsOpen(!editFolderModalIsOpen);

<Image
className={styles.optionBtnImg}
src={deleteImg}
alt="deleteImg"
Copy link
Collaborator

Choose a reason for hiding this comment

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

alt는 한국어로 편하게. 그리고 그림을 설명하듯 작성해주시면 됩니다 !

alt는 스크린 리더 사용자에 대한 보조 텍스트가 될 수 있으므로 "어떠한 이미지 인지"를 작성해주는 것이 좋아요 !

alt의 목적

  • 인터넷 연결이 끊겼을 때 대체되는 이미지
  • 스크린 리더 사용자를 위한 대체 텍스트
  • 이미지를 볼 수 없는 환경에서 이미지를 대체하기 위한 텍스트
    등 목적을 알게 된다면 alt를 어떻게 사용하시면 될지 알 수 있을 것 같아요.

다음은 하버드 에듀케이션에서 제안하는 alt 규칙입니다:

tl;dr

  • Write Good Alt Text
  • Add alt text all non-decorative images.
  • Keep it short and descriptive, like a tweet.
  • Don’t include “image of” or “photo of”.
  • Leave alt text blank if the image is purely decorative
  • It's not necessary to add text in the Title field.

원문 보기

Comment on lines +19 to +36
useEffect(() => {
const fetchProfileInfo = async () => {
try {
const response = await fetch(
'https://bootcamp-api.codeit.kr/api/users/1'
);
if (!response.ok) {
throw new Error('response 전달 실패');
}
const data = await response.json();
setProfileData(data.data[0]);
} catch (error) {
console.error('에러 발생:', error);
alert(error);
}
};
fetchProfileInfo();
}, []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 값도 커스텀 훅으로 분리해볼 법 하군요 🤔

또한 모든 base url 값들을 환경 변수로 변경해보는게 어떨가 싶습니다 !

@kiJu2
Copy link
Collaborator

kiJu2 commented May 15, 2024

수고하셨습니다 채민님 !!
채민님의 도전성이 보이는 코드였습니다 ㅎㅎㅎ 다음 코드 또한 기대가 되는데요.
비록 리액트 쿼리를 사용하지는 못하였으나 팀 프로젝트에서 충분한 경험이 가능할거라고 봅니다 😊😊

항상 멘토링 미팅 활발하게 활동해주셔서 진심으로 감사드립니다.

코드리뷰 중 궁금한 점 있으시면 사전 질의를 통해 편하게 물어봐주세요 !

@kiJu2
Copy link
Collaborator

kiJu2 commented May 15, 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.

3 participants