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

7, 8주차 코드리뷰 #73

Merged
merged 5 commits into from
Oct 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .env.development
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
VITE_BASE_URL=/
VITE_GOOGLE_AUTH_CLIENT_ID=268837811477-28b8i24r1sb0aroltho84ia6jecj74h7.apps.googleusercontent.com
VITE_GOOGLE_AUTH_REDIRECT_URI=http://localhost:5173/loading
43 changes: 43 additions & 0 deletions .github/workflows/lint.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
name: Lint and Type Check
Copy link
Contributor

Choose a reason for hiding this comment

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

보통 이런걸 ci 라고 한답니다!


on:
pull_request:
# 모든 브랜치에 대해 PR 시 실행
branches:
- '**'
push:
# 모든 브랜치에 대해 푸시 시 실행
branches:
- '**'
Comment on lines +8 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

push에 대해서 실행하면... 비용이 너무 많이 들 것 같아요 ㅎㅎ
pr의 sync에 대해서 실행해주면 어떨까요?

on:
  pull_request_target:
    types:
      - synchronize
      - opened


jobs:
lint:
name: Lint and Type Check
runs-on: ubuntu-latest

steps:
# Checkout the repository
- name: Checkout repository
uses: actions/checkout@v3

# Set up Node.js environment
- name: Set up Node.js
uses: actions/setup-node@v3
with:
node-version: '18'

# Install dependencies
- name: Install dependencies
run: npm ci

# Run ESLint to check for linting errors
- name: Run ESLint
run: npm run lint

# Run Prettier to check formatting
- name: Run Prettier Check
run: npm run format -- --check

# Run TypeScript compiler to check for type errors
- name: Run TypeScript Check
run: npm run tsc
Comment on lines +33 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

이렇게 세 개는 husky 같은걸 통해서 git에 push하기 전에 로컬에서 스테이지된 파일에 대해 검사하는 방법도 있답니다!

git hook 이라는 키워드로 한 번 찾아보세요~

17 changes: 14 additions & 3 deletions .storybook/preview.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import React from 'react';
import type { Preview } from '@storybook/react';
import AppProviders from '../src/components/providers/index.provider';
import { initialize, mswLoader } from 'msw-storybook-addon';
import { handlers } from '../src/mocks/handlers';
import { MemoryRouter } from 'react-router-dom';

initialize();

const preview: Preview = {
parameters: {
Expand All @@ -10,15 +15,21 @@ const preview: Preview = {
date: /Date$/i,
},
},
msw: {
handlers: [...handlers],
},
},
tags: ['autodocs'],
decorators: [
(Story) => (
<AppProviders>
<Story />
</AppProviders>
<MemoryRouter>
<AppProviders>
<Story />
</AppProviders>
</MemoryRouter>
),
],
loaders: [mswLoader],
};

export default preview;
88 changes: 53 additions & 35 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,42 +1,60 @@
# 🍪 내가 먹은 쿠키 - 18조 FE

## 🙋‍♂️ 6주차 코드리뷰 질문
- 하나의 파일 내에서 함수 표현식과 선언식을 같이 사용해도 되는지 궁금합니다.
- 기존 Weekly 브랜치에 components 경로에 `RoleModal` 컴포넌트가 있다고 가정하면, 해당 Weekly브랜치를 통해 새로 분기된 Feat 브랜치에 기존 components에 `RoleModal` 컴포넌트를 features 경로에 옮기고 두 브랜치를 머지하게되면 두개의 파일이 생기게 됩니다. 이를 해결하려면 어떻게 해야하나요?
## 🙋‍♂️ 7, 8주차 코드리뷰 질문
- MyRecruitList에서 ‘내가 지원한 공고’ 목록의 버튼 디자인은 API로 전달받는 값에 따라 달라집니다. 이를 반영하기 위해 getStateStyle 함수를 만들었고, 함수의 파라미터로는 API로 전달받는 값인 변수 state를 사용하고, 함수의 반환값은 버튼의 design과 버튼 내부 text로 설정하였습니다.
```tsx
type MyRecruitListProps = {
title: string;
address: string;
salary: string;
image: string;
state: string;
};

type designProps = {
design: 'default' | 'outlined' | 'textbutton' | 'deactivate';
text: string;
};

## 🙋‍♂️ 5주차 코드리뷰 질문
- 하나의 페이지 내에서만 여러번 사용되는 공통 컴포넌트의 경우, components/common 폴더에 공통 컴포넌트로 만들어 취급하는 것이 좋은지, 혹은 해당 페이지 코드 파일이 위치한 폴더에 컴포넌트를 만들거나 해당 페이지 코드 파일 하단에 작성하는 등 colocation 원칙을 적용해서 가까이 위치시키는 것이 좋을지 궁금합니다.
- `Header` 컴포넌트에서 다른 theme을 가진 버튼들에 공통된 스타일을 적용하면서, 특정 버튼에만 추가적인 스타일을 주는 작업을 했습니다. 아래와 같이 각 버튼에 공통적으로 적용될 스타일을 `commonButtonStyles`로 정의하고, `theme=default`인 버튼에만 추가 스타일을 적용해보았는데, 제가 구현한 방식보다 더 괜찮은 방법이 있는지 궁금합니다.
```jsx
const commonButtonStyles = css`
white-space: nowrap;
border-radius: 4px;
`;
function getStateStyle(state: MyRecruitListProps['state']): designProps {
switch (state) {
case '근로계약서 서명하기':
return { design: 'default', text: '근로계약서 서명하기' };
case '채용 마감':
return { design: 'deactivate', text: '채용 마감' };
case '지원서 검토중':
return { design: 'outlined', text: '지원서 검토중' };
case '채용 완료':
return { design: 'deactivate', text: '채용 완료' };
default:
return { design: 'deactivate', text: '알 수 없음' }; // 상태가 정의되지 않은 경우
}
}
```

그런데 desginProps의 design은 공통컴포넌트로 만들었던 button의 design 프롭스로 전달되는 값으로, 이미 button 컴포넌트 내부에서 design 도메인을 정의한 바가 있습니다. 만약 위 코드처럼 제가 작성한대로 한다면, button 컴포넌트에서 design부분을 수정한다면 위 코드도 직접 수정해야하는 불편함이 있을것 같다고 생각합니다. 혹시 공통컴포넌트에서 정의한 프롭스 도메인을 다른 부분에서 재사용할 수 있는 좋은 방법이 있을까요?
getStateStyle의 인자로 받는 state는 MyRecruitListProps의 state와 같은 값을 공유하므로 위와 같이 작성했는데, 혹시 미리 정의한 프롭스 타입의 일부분을 사용하는 더 좋은 방법이 있을까요?
아래와 같이 props 타입을 지정하고, 아래 타입을 가진 데이터를 mock데이터로 받아오는 코드를 작성했습니다.

```tsx
export type StateProps = '근로계약서 서명하기' | '채용 마감' | '지원서 검토중' | '채용 완료';
<MyRecruitList myRecruitList={myRecruitList} />
```
<Button theme="outlined" css={[commonButtonStyles]}>
채용공고 등록
</Button>
<Button theme="textbutton" css={[commonButtonStyles]}>
닉네임
</Button>
<Button
css={[
commonButtonStyles,
css`
background-color: #0a65cc;
color: #fff;
`,
]}
>
로그아웃
</Button>
위 코드에서 {myRecruitList}가 mock데이터인데, 이때 이 데이터가 타입에 맞지 않는다고, 특히 state 부분이 StateProps 값을 기대하는 반면 string 값이 들어온다고 타입오류가 발생합니다. 그래서 mock데이터 자체에 아래와 같이 타입을 지정하니 오류는 사라졌는데, 이부분에 진짜 API 데이터를 연결하게 되어도 안전할지 모르겠습니다.
```tsx
export const myRecruitList: MyRecruitListProps[] = [
{
id: 1,
image:
'https://img.freepik.com/free-photo/low-angle-view-of-skyscrapers_1359-1105.jpg?size=626&ext=jpg&ga=GA1.1.1297763733.1727740800&semt=ais_hybrid',
title: '제목',
area: '대전광역시 유성구',
state: '근로계약서 서명하기',
},
...
```
- 태블릿(`768px`)과 모바일(`480px`)에서 반응형을 고려하여 `breakpoints`를 정의하였고, 이를 보다 명시적으로 활용하기 위해 `responsiveStyles` 함수를 구현했습니다.
멘토님께서는 보통 반응형 스타일링을 구현할 때 어떤 방식을 사용하시나요?
혹시 제가 사용한 `responsiveStyles` 함수보다 효율적이거나 코드의 가독성을 높일 수 있는 더 나은 방법이 있을까요? 멘토님이 추천하는 방법이나 일반적으로 사용되는 best practice 또한 궁금합니다.
- 현재 `Modal` 컴포넌트를 사용할 때마다 `useToggle` 커스텀 훅을 함께 사용해야 해서, 모달을 제어하기 위한 코드가 흩어져 있는 느낌입니다. 이렇게 되면 모달 관련 로직으로 인해서 단일 책임 원칙에 어긋난다는 생각이 들곤 합니다.
보다 나은 방식으로 `Modal` 컴포넌트를 동작시킬 수 있는 방법이 있을까요? `useToggle`처럼 모달을 제어하는 로직을 간소화하고, 모달 컴포넌트 자체가 스스로 상태를 관리하거나 쉽게 제어 가능한 형태로 구현할 수 있는지 궁금합니다.

## 🙋‍♂️ 4주차 코드리뷰 질문
- `Modal` 컴포넌트를 구현하면서 텍스트 부분과 버튼 부분에 들어갈 내용은 개발할 때 코드를 작성하는 사람이 자유롭게 작성하여 구성할 수 있도록 `textChildren`과 `buttonChildren`만으로 구성하였는데, 더 적합하거나 지향하는 방식이 있을까요
이처럼, 여러 값만 도메인으로 설정한 타입들에 한해 자주 에러가 발생하는데 이런 부분을 어떻게 해결하면 좋을까요?

- 고용주 마이페이지, 내 회사 페이지, 지원자 목록 페이지에 공통으로 사용되는 테이블 컴포넌트를 구현했습니다. 세 페이지 모두 같은 스타일의 테이블을 사용하여 구현 시 스타일을 컴포넌트 자체에 내장하여 일괄적으로 적용되도록 했는데, 이런 방식으로 사용해도 될지 궁금합니다. 스타일을 고정하여 일관성을 유지하는 것과 사용 시점에 스타일을 적용해서 유연성을 제공하는 방법 중에 어느 것을 선택하는 것이 좋을까요?

2 changes: 2 additions & 0 deletions eslint.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ export default tseslint.config(
...reactHooks.configs.recommended.rules,
'react-refresh/only-export-components': ['warn', { allowConstantExport: true }],
'react-refresh/only-export-components': 'off',
'react-hooks/rules-of-hooks': 'off',
'@typescript-eslint/no-explicit-any': 'off',
},
},
);
Loading
Loading