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

[Park] AvatarGroup 내부 로직 리팩토링 (cloneElement, Children 관련) #61

Merged
merged 3 commits into from
May 2, 2023

Conversation

healtheloper
Copy link
Member

close #60

이번에 리액트 문서(Children, cloneElement) 를 읽어보면서, 제안하고 있는 대체 방법으로 우리의 코드를 개선할 수 있을지 고민해보고, 적용해보았습니다.

Children 관련 문서를 읽으면서, children 으로 인한 문제가 생기지 않겠다는 보장은 어렵다. (사용자가 avatars 를 fragment 로 감싸서 보내준다면, total(Children.count) 이 1개로 적용될 것이고, Children 관련 유틸함수가 제대로 동작하지 않을 것

-> 어차피 Avatar 에 커플링되는 컴포넌트라면 AvatarProp 형태의 객체배열을 받아 사용하는게 낫다고 생각하였고, Children 관련 util 을 모두 제거하고, avatars 를 통해 아바타 관련 객체배열을 전달받아 사용하는 것으로 대체하였습니다.

cloneElement 를 사용하는 것이 데이터의 흐름(여기선 스타일 관련 props 주입)과 child 의 추적이 어렵다는 단점을 꼽을 수 있겠는데, AvatarGroup 은 Avatar 와 연결되어있다라는 것이 당연한게 아닌가? 라는 생각이라서, child 가 무엇인지 알기 어렵다는 점은 무시할 수 있다고 생각하고, AvatarGroup 을 사용하는 사용자가 굳이 알아야 할 필요도 없는 로직이라고 생각했습니다.

-> 사용자가 Avatar 컴포넌트를 직접 사용하여 넣는방식 대신 AvatarGroup 컴포넌트 내부에서 풀어 사용하면 굳이 cloneElement 를 사용하지 않아도 되겠다고 생각하여 대체하였습니다.

이 외에도 중복된 로직이라고 생각되는 부분을 isOverMax 등의 변수를 활용하여 제거하였습니다.

Copy link
Contributor

@hemudi hemudi left a comment

Choose a reason for hiding this comment

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

내부 구조를 이미 이해하고 있어서 그런가 기존 코드의 흐름이 어렵다거나 혼란하다고 생각하지 못했었는데 제거 되고 보니 코드가 좀 더 명시적인 느낌이 드네요ㅎㅎ
수고하셨습니다! LGTM! ^_^)b

Comment on lines 10 to +12
- `max` : `Avatar` 가 보여질 최대 갯수를 지정합니다. `default: 5`
- `spacing` : `Avatar` 간의 간격을 `small`, `medium`, `large` 중 선택할 수 있습니다. `default: small`
- `size` : `Avatar` 들의 사이즈를 지정합니다. `default: small`
Copy link
Contributor

Choose a reason for hiding this comment

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

props 설명에 avatars 가 추가 되면 좋을 것 같아요!
아래 avatarGroup.tsx 파일엔 @props avatars : Avatar 의 정보를 담는 배열을 입력합니다. ex) AvatarProps[] 라고 추가되어 있네요!

@healtheloper healtheloper changed the title AvatarGroup 내부 로직 리팩토링 (cloneElement, Children 관련) [Park] AvatarGroup 내부 로직 리팩토링 (cloneElement, Children 관련) May 2, 2023
@healtheloper healtheloper merged commit b3a45c9 into dev May 2, 2023
@healtheloper healtheloper deleted the refactor/avatar-group/#60 branch May 2, 2023 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AvatarGroup 내부 로직 관련(cloneElement, Children)
2 participants