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.

Sign up for GitHub

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

Jump to bottom

fix: Use styled props in Collapse component #1

Merged
merged 2 commits into from May 10, 2022
Merged

fix: Use styled props in Collapse component #1

merged 2 commits into from May 10, 2022

Conversation

mu-hun
Copy link
Contributor

@mu-hun mu-hun commented May 8, 2022 •
edited

리액트에서 권장하는 "진리의 원천"(Single source of truth) 개념을 바탕으로 꼭 필요하고 최소한의 props 의 조합으로 새 속성 값을 할당하는 방법을 사용했습니다:

어떤 값이 props 또는 state로부터 계산될 수 있다면, 아마도 그 값을 state에 두어서는 안 됩니다.

교훈, State 끌어올리기 – React

로컬에서 스토리북을 실행해서 컴포넌트가 이상 없는지 확인을 마쳤습니다..!

@mu-hun
fix: Use styled props in Collapse component
21be11c 
리액트에서 권장하는 "진리의 원천" 개념을 바탕으로 최소한의 props 의 조합으로 새 속성 값을 할당하는 방법을 사용했습니다:

> _어떤 값이 props 또는 state로부터 계산될 수 있다면, 아마도 그 값을 state에 두어서는 안 됩니다._
>
> [교훈, State 끌어올리기 – React](https://ko.reactjs.org/docs/lifting-state-up.html#lessons-learned)
mu-hun
mu-hun commented May 8, 2022
View reviewed changes
src/components/Collapse.tsx Outdated Show resolved Hide resolved
@mu-hun
Copy link
Contributor Author

mu-hun commented May 8, 2022 •
edited

수정 이후에 타입 검사를 거치고 싶었는데요, 타입 체킹을 아직 정리 중이신 걸로 보여서 이전 소스로 체크아웃한 로그와 차이점이 없는지 직접 diff 를 따서 비교해보았습니다 😂

Found 182 errors in 6 files.

6개의 파일에 182 오류가 있고 수정 전에서부터 동일하게 있는 문제로 확인했습니다.

shiftpsh
shiftpsh requested changes May 8, 2022
View reviewed changes
Copy link
Member

@shiftpsh shiftpsh left a comment

Choose a reason for hiding this comment

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

기여해 주셔서 감사합니다! Collapse 같은 경우에는 제가 다른 프로젝트(아마도 shiftpsh.com)에서 만든 것을 그대로 가져온 거라 제대로 신경쓰지 못했다 봅니다..

src/components/Collapse.tsx Outdated Show resolved Hide resolved
@shiftpsh
Copy link
Member

shiftpsh commented May 8, 2022

앗 추가로 저는 test:lint의 경우 딱히 에러가 있다고 하진 않는데 혹시 어떤 방법으로 타입 체킹을 해주셨나요 ?ㅁ?

@mu-hun
Copy link
Contributor Author

mu-hun commented May 9, 2022

tsc --noEmit 로 검사를 했습니다. 수정 전후 오류 갯수가 같아서 타입 미스가 없는 걸로 확인을 했습니다.

Found 182 errors in 6 files.

@mu-hun
chore: Rename to CollapseContainer
f946f76 
@x86chi x86chi yesterday

혹시 RenderComponent 라는 이름이 DOM 커스텀 태그를 위한 네이밍인가요?

제가 "as" polymorphic prop 로 교체하면서 해당 용도가 없어졌는데요, 다른 컴포넌트의 컨테이너 네이밍을 따라 CollapseContainer 로 바꾸면 어떨까 싶습니다..!

@shiftpsh shiftpsh 13 hours ago

괜찮은 것 같습니다. 혹시 그렇게 수정해주실 수 있으신가요?

ref: solved-ac#1 (comment)
@mu-hun mu-hun requested a review from shiftpsh May 9, 2022 13:06
@mu-hun
Copy link
Contributor Author

mu-hun commented May 9, 2022

수정 요청 반영했습니다!

shiftpsh
shiftpsh approved these changes May 10, 2022
View reviewed changes
Copy link
Member

@shiftpsh shiftpsh left a comment

Choose a reason for hiding this comment

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

감사합니다!

@shiftpsh
Copy link
Member

검사하는 방법을 몰라서 안 하고 있었는데 한번 검사 돌려보고 오류 잡아봐야겠습니다. 기여 감사합니다!

@shiftpsh shiftpsh merged commit 609190f into solved-ac:main May 10, 2022
@mu-hun mu-hun deleted the fix/use-styled-props branch May 10, 2022 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shiftpsh shiftpsh shiftpsh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mu-hun @shiftpsh