Skip to content

TypeScript + React-Redux + AtomicPattern#4

Open
DDuckyee wants to merge 3 commits intomainfrom
develop
Open

TypeScript + React-Redux + AtomicPattern#4
DDuckyee wants to merge 3 commits intomainfrom
develop

Conversation

@DDuckyee
Copy link
Copy Markdown
Owner

Title is detail

Copy link
Copy Markdown
Collaborator

@jaeseo222 jaeseo222 left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다! 협업 시, 일관성을 위해 코드컨벤션 위주로 남겼습니다!

사실 현재와 같은 컴포넌트 디자인 패턴으로써 로직과 view로 나뉜 presentational and container는 추천하지 않는 것으로 알고 있어요. (참고1) (참고2)

하지만,, Redux를 활용 중인 StartupQT 개발 하실 땐 이미 개발해 놓은 구조가 있기 때문에, 이대로 개발해주시면 될 것 같아요! 아님 .. 나중에 대공사를 하거나...

로직과 view를 분리하는 건 presentational and container 대신, Hook로도 가능한데
이걸 zustand가 제공하고 있기 때문에 LALA에선 container 없이 구현해주시면 될 것 같습니다. 😊

Comment on lines +3 to +6
type CounterButtonProps = {
editCount: () => void;
editValue: string;
};
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

작성해주신 type도 상관없는데, 전 LALA에서 interface로 정의했어요!
주석님도 LALA에서는 일관성을 위해 interface로 맞춰주시면 좋을 것 같아요!

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

넵, interface로 통일하겠습니다 :)

Comment on lines +9 to +11
function CounterButton(props: CounterButtonProps) {
return <button onClick={props.editCount}>{props.editValue}</button>;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

작성해주신 방법처럼 props 로 받는 것도 좋은데
저는 LALA에서 보통 props.editCount 로 접근하지 않고,
CounterButton({editCount, editValue}) 혹은
const {editCount, editValue} = props 처럼 editCount에 바로 접근하는 식으로 구현했습니다! 이 부분도 LALA에서는 일관성을 위해 맞추면 좋을 것 같아요!

Copy link
Copy Markdown
Owner Author

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 +13
onClick={(event) => {
event.preventDefault();
props.onChangeMode();
}}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

onClick 이벤트 함수 같은 경우엔, 저번에 피드백 남겼던 것처럼 따로 함수로 빼주는 게 좋을 것 같습니다! (재사용성, 무슨 기능인지 명시를 위해)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

아, 해당 부분 수정을 안했었네요.
추후 개발때는 수정하겠습니다.

Comment on lines +23 to +38
const lis = props.topics.map(function (element) {
return (
<li key={element.id}>
<a
id={element.id}
href={"/read/" + element.id}
onClick={(event) => {
event.preventDefault();
props.onChangeMode(Number(event.target.id));
}}
>
{element.title}
</a>
</li>
);
});
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

저는 map 사용할 때 arrow function으로 구현했습니다!
이 부분도 LALA에서는 일관성을 위해 맞추면 좋을 것 같습니다.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

이 부분도 element.title 대신, title 에 바로 접근하는 걸로 맞추면 좋을 것 같아요! (다른 컴포넌트에서도 마찬가지에욥!)

작성해주신 방법처럼 props 로 받는 것도 좋은데
저는 LALA에서 보통 props.editCount 로 접근하지 않고,
CounterButton({editCount, editValue}) 혹은
const {editCount, editValue} = props 처럼 editCount에 바로 접근하는 식으로 구현했습니다! 이 부분도 LALA에서는 일관성을 위해 맞추면 좋을 것 같아요!

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

네, arrow function으로 통일하겠습니다.
해당 부분도 그렇게 진행하겠습니다 :D

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