-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Feature/BAR-6] WriteInput 작성 #8
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사소한 컨벤션 코멘트 남겨두었습니다! 수고하셨습니당
import type { ChangeEvent } from 'react'; | ||
import { useState } from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dongkyun-dev
이후에 같은 경로에서 import될 때 type을 inline으로 작성할 지 아니면 위처럼 분리해 작성할지
한 가지 방식으로 강제하는 rule을 추가하면 좋을 것 같습니다!
현재는 두가지 방식 다 작성 가능하도록 되어있더라고요.
import type { ChangeEvent } from 'react'; | |
import { useState } from 'react'; | |
import { useState, type ChangeEvent } from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wonjin-dev @dmswl98
좋아요 혹시 선호하시는 방식 있으신가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dongkyun-dev 저는 현재 코드에서 많이 사용된 방식으로 하면 좋을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmswl98
저는 개인적으로 분리하지 않고 하나의 라인에서 작성하는게 좋긴 한데....! 관련해서 린트룰을 찾아볼까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dongkyun-dev 그럼 이렇게 작성하시면 됩니다!!
'@typescript-eslint/consistent-type-imports': [
'error',
{ fixStyle: 'inline-type-imports' },
],
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmswl98
오오 혹시 반영해서 PR 작성해주실 수 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
린트 룰 추가하고 반영할때 일괄 fix 가시죵
base: [ | ||
{ | ||
display: 'flex', | ||
alignItems: 'flex-end', | ||
paddingLeft: '20px', | ||
}, | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
base: [ | |
{ | |
display: 'flex', | |
alignItems: 'flex-end', | |
paddingLeft: '20px', | |
}, | |
], | |
base: { | |
display: 'flex', | |
alignItems: 'flex-end', | |
paddingLeft: '20px', | |
}, |
이렇게 작성해주셔도 됩니다!
}; | ||
|
||
export const Icon: IconFactory = { | ||
export const iconFactory = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상수는 모두 UPPER_CASE
로 작성해주시면 감사하겠습니당
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요건 하드 코딩이 필요한 상수가 아니여서 변수로 사용하는 부분이어서 이렇게 처리하였습니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셧습니다!
간단히 수정되면 좋을 부분만 커멘트 남겨두엇습니다 ㅎㅎ
import WriteInput from '@/src/components/Input/WriteInput'; | ||
import { useInput } from '@/src/hooks/useInput'; | ||
|
||
const HomePage: NextPage = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NextPage
와 같은 타입 명시가 필요한가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
자동으로 추론하지 못하기 때문에 필요합니당 !
maxLength = MAIN_INPUT_MAX_LENGTH, | ||
}: WriteInputProps) => { | ||
const { id, value } = inputProps; | ||
const inputRef = useRef<HTMLTextAreaElement | null>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| null
은 제거되어도 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
초기 값을 위해 필요한 부분입니다.
src/hooks/useInput.ts
Outdated
} | ||
|
||
export const useInput = ({ id, defaultValue = '' }: UseInputArgs) => { | ||
const [value, setValue] = useState<string>(defaultValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<string>
제네릭도 없어도 좋을 것 같습니다!
Summary
To Reviewer
How To Test
브랜치 체크 아웃
이후메인 페이지
에서 확인