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

전남대 FE_서민지_1주차 과제 #72

Open
wants to merge 41 commits into
base: minji2219
Choose a base branch
from

Conversation

minji2219
Copy link

안녕하세요 멘토님. 1주차 과제 입니다. 다름이 아니라 과제 ESLint 설정에 대해 몇가지 질문 드리려고 합니다!

.eslintrc.json 파일을 생성하여 린트 적용을 하기 위해서는 추가 작업이 필요하나요?
파일 생성하지 않고 cra할때 생성되는 package.json 내부의 lint코드는 문제 없이 돌아가는데
해당 내용을 지우고 .eslintrc.json 파일을 생성하여 lint를 적용하면 lint가 적용이 되지 않았습니다.
그래서 이것 저것 플러그인을 설치했는데, 어떤 플러그인들이 필요한 플러그인들이고 어떤 것이 불필요한 것인지 궁금합니다.
또, lint와 prettier 겹치게 되면 lint가 무시되고, prettier로 따라가는게 맞나요..?

추가로 components폴더에서 실제 component와 story는 폴더로 구성하는게 더 좋은 구성인가요?
ex. components/button/Button.tsx와 components/button/Button.stories.tsx

minji2219 and others added 30 commits June 25, 2024 22:57
CRA로 REACT구축시 웹팩 설정 때문에 tsconfig를 변경한 내용이 적용되지 않고 초기 생성된 tsconfig 설정으로 돌아가기 때문에 craco를 사용하여 웹팩 설정을 변경해줌
*import React하지 않아도 오류 뜨지 않게 설정하기 위해
*interface에서 매개변수 타입 지정시 unused-var error뜨지 않게 하기 위해
Input 컴포넌트 내에서 onChange는 필수적이기에 추가 및 storybook에 해당 기능 추가
*src, alt, width, ratio, radius를 props로 받음
*BigImage
*SmallImage(circle)
*Default
*Ranking
*primary -> kakao
*onClick이벤터 핸들러 추가
*style수정
@tlsehdfl
Copy link

tlsehdfl commented Jul 3, 2024

안녕하세요. FE 멘토를 담당하게 된 신동리 입니다.
우선 리뷰가 늦어져서 죄송합니다.
앞으로 잘 해봐요 🙏🏻

import { css } from '@emotion/react';
import { ReactNode } from 'react';

interface ButtonProps {
Copy link

Choose a reason for hiding this comment

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

html button element 의 기본 properties 를 상속하는 방식으로 ButtonProps 를 구현하는게 확장성을 고려했을 때 좀 더 유려합니다.

예를 들면, 아래와 같습니다.

// e.g.
interface ButtonProps extends ComponentProps<'button'> {
  type?: 'button' | 'submit' | 'reset';
  theme?: string;
}

interface ButtonProps {
width?: number;
height?: number;
theme?: string;
Copy link

Choose a reason for hiding this comment

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

theme 을 string 타입으로 받기 보다는 몇 가지 정의된 타입으로 지정하는게 type narrowing 측면에서 좋습니다.

// e.g.
theme?: 'kakao' | 'danger';

Copy link

Choose a reason for hiding this comment

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

children,
} = props;

let themeBtn;
Copy link

Choose a reason for hiding this comment

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

if-else 문을 사용하는 거 보다는 object literals 로 작성하는 것이 가독성 측면에서 좀 더 좋습니다.

// e.g.
const THEME = {
  kakao: css`background-color: #fee500; `;
  danger: css`background-color: red; color: white;`;
}

const styles = THEME[theme];

Copy link

Choose a reason for hiding this comment

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

| 'space-around';
alignItems?: 'center' | 'flex-start' | 'flex-end' | 'baseline' | 'stretch';
}
const Container = (props: ContainerProps) => {
Copy link

Choose a reason for hiding this comment

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

해당 컴포넌트를 재사용하기 위하여 공통 컴포넌트로 작성하는건 불필요해 보입니다.
단순하게 div element 로 사용해도 될 거 같아요.

@tlsehdfl
Copy link

tlsehdfl commented Jul 3, 2024

Q ) .eslintrc.json 파일을 생성하여 린트 적용을 하기 위해서는 추가 작업이 필요하나요?
A ) 처음부터 일일이 설정하려면 어려우실 수 있습니다.

npx eslint --init

로 초기화 한 이후에 필요한 설정만 변경 & 추가 하는 방식을 추천드려요.

Q ) lint와 prettier 겹치게 되면 lint가 무시되고, prettier로 따라가는게 맞나요..?
A ) eslint / prettier 는 역할이 다릅니다.

  • eslint는 javascript 에서 발생할 수 있는 오류나 잠재적인 문제를 검사하는 분석 도구로 코드 품질을 향상시키고 버그를 사전에 예방해줍니다.
  • prettier는 코드 포맷팅 도구로 자동으로 코드를 정리하고 일관된 스타일로 포맷팅해준다.

eslint / prettier 가 충동하는 경우는 https://velog.io/@leejungho9/Error-ESLint-Prettier#eslint%EC%99%80-prettier-%EC%B6%A9%EB%8F%8C-%ED%95%B4%EA%B2%B0-%EB%B0%A9%EB%B2%95 해당 글을 참고 바랍니다.

Q ) components폴더에서 실제 component와 story는 폴더로 구성하는게 더 좋은 구성인가요?
A ) 현재 작성해주신 구성이 괜찮아보입니다. component 코드가 pair 로 작성하여 .story.tsx 파일만 불러오게끔 설정할 수 있습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants