-
Notifications
You must be signed in to change notification settings - Fork 20
[유선향]- sprint6 #61
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
[유선향]- sprint6 #61
The head ref may contain hidden characters: "React-\uC720\uC120\uD5A5-sprint6"
Conversation
|
스프리트 미션 하시느라 수고 많으셨어요. |
프로젝트 이후로 제가 한 스프린트 미션을 보니 참... 정리할 부분이 많은것 같아서 정리하느라 (파일구조 변경등...)변경사항이 좀 많습니다.🥲긍정적인 소식이네요 ㅎㅎㅎ 기초 프로젝트를 수행하시면서 안목이 트였기에 보이지 않았던 부분이 보이는거라 생각됩니다 ! 앞으로도 함께 더 devel-up ! 합시다 ! 😊 다 정리 하진 못해서 input.jsx , input.style.jsx , additem.jsx , addItem.style.jsx ,tag, tag.style.이 여섯가지 파일이 이번에 진행한 파일인점 참고 해주시면 감사하겠습니다!정리 감사드립니다 ! 해당 파일 위주로 코드리뷰 하도록 할게요 ! |
| import * as S from "./Input.style"; | ||
| import { useRef, useState } from "react"; | ||
| // | ||
| export function Input({ label, placeholder, name, onChange, ...props }) { |
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.
크으 ~ 훌륭하네요.
...props로 해두셨으니 해당 공통 컴포넌트를 수정할 일이 적어지고 확장성이 좋아질 수 있겠군요 ! 👍👍
| import { useRef, useState } from "react"; | ||
| // | ||
| export function Input({ label, placeholder, name, onChange, ...props }) { | ||
| const { tag, onKeyUp, value, type, textArea, ...rest } = props; |
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.
(제안): TagInput를 새로 만들어도 될 것 같아요.
Input 컴포넌트는 범용적인 입력 필드여야 하지만, tag 관련 로직이 포함되어 있어 역할이 모호해질 것으로 사료되어 제안드려봐요 ㅎㅎㅎ
선향님께서 만드신 것처럼 다음과 같이 Input은 다음과 같이 작성하고 TagInput은 따로 만들어 볼 수 있어요:
export function Input({ label, placeholder, name, onChange, onKeyUp, value, type = "text", textArea, ...rest }) {
return (
<S.InputWrapper>
{label && <S.Label>{label}</S.Label>}
<S.Input
type={type}
value={value}
name={name}
$textArea={textArea}
placeholder={placeholder}
onChange={(e) => onChange?.(e.target.value)}
onKeyUp={onKeyUp}
{...rest}
/>
</S.InputWrapper>
);
}| const handleOnKeyUp = (e) => { | ||
| onKeyUp(e); | ||
| }; |
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.
handleOnKeyUp은 제외해도 되지 않을까요 ?
| const handleOnKeyUp = (e) => { | |
| onKeyUp(e); | |
| }; | |
| const handleOnKeyUp = (e) => { | |
| onKeyUp(e); | |
| }; |
별 다른 기능이 없어보입니다 !
만약 필요하게 된다면 ...props에 포함되어 정상동작할 것으로 예상됩니다 !
사실 마찬가지로 handleChange도 e.target이 필요가 없도록 설계된다면 필요 없을 것으로 사료됩니다 !:
const handleChange = (e) => {
onChange(e.target);
};| type={type ? type : "text"} | ||
| value={value} | ||
| name={name} | ||
| $textArea={textArea} | ||
| placeholder={placeholder} |
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.
rest에 이미 포함되어있으므로 제거하셔도 정상동작 할 것으로 예상됩니다 !
| type={type ? type : "text"} | |
| value={value} | |
| name={name} | |
| $textArea={textArea} | |
| placeholder={placeholder} | |
| $textArea={textArea} |
위처럼 작성하고:
export function Input({ label, onChange, ...props })
const { tag, onKeyUp, textArea, ...rest } = props;위와같이 작성될 수 있겠네요 !
| const INITIAL_DATA = { | ||
| img: "", | ||
| name: "", | ||
| content: "", | ||
| price: 0, | ||
| tags: [], | ||
| }; |
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.
초기값이 길어질 수 있으니 상수로 따로 선언하셨군요? 😊
배려심이 돋보입니다 !
| setFormData((prev) => ({ ...prev, tags: filterTags })); | ||
| }; | ||
|
|
||
| const requiredInput = ["name", "content", "tags", "price"]; |
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.
해당 값도 컴포넌트 바깥에 상수로 지정해도 되겠네요 😊
| export default function Tag({ value, onClick }) { | ||
| const handleClickDeleteBtm = () => { | ||
| onClick(value); | ||
| }; | ||
| return ( | ||
| <S.Container> | ||
| <S.FlexContents> | ||
| <S.Tag>#{value}</S.Tag> | ||
| <S.DeleteButton onClick={handleClickDeleteBtm} src={DeleteButton} /> | ||
| </S.FlexContents> | ||
| </S.Container> | ||
| ); | ||
| } |
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.
컴포넌트 단위가 참 좋네요 ㅎㅎㅎ 🤣
만약, TagInput이 생겨난다면 TagInput.jsx 파일 내에 export하지 않은 private한 컴포넌트로 자리잡을 수도 있겠군요 !
|
크으 ~ 수고하셨습니다. 정말 수고 많으셨습니다 선향님 ! 👍👍 |
요구 사항
기본
심화
주요 변경사항
-install lodash.throttle
스크린샷
멘토에게
-프로젝트 이후로 제가 한 스프린트 미션을 보니 참... 정리할 부분이 많은것 같아서 정리하느라 (파일구조 변경등...)변경사항이 좀 많습니다.🥲