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

Feat/icon: Icon 컴포넌트 deprecate 및 디자인 시스템 아이콘 추가 #110

Merged
merged 6 commits into from
Apr 24, 2023
Merged

Feat/icon: Icon 컴포넌트 deprecate 및 디자인 시스템 아이콘 추가 #110

merged 6 commits into from
Apr 24, 2023

Conversation

se030
Copy link
Contributor

@se030 se030 commented Apr 22, 2023 •
edited
Loading

🤠 개요

💫 설명

작업에 강조해야 할 내용은 없어 스토리북 캡쳐로 대체합니다.

  • 아이콘들은 로드 요청 없이 사용할 수 있도록 인라인 svg로 가져왔어요.

  • 네이밍은 react-icons 따라했어요.

image

Icon 컴포넌트를 폐기해도 될까요 ..?

  • Icon이 만들어진건 초반에 Button, 그 외 컴포넌트를 개발하면서 icon props로 받은 react-icons.IconType과 이미지 소스 URL을 동일한 컴포넌트로 추상화하고 싶어서였어요.

  • 그래서 배럴 파일에도 포함되지 않은 단순 개발용 컴포넌트에요.

  • Button 컴포넌트 리팩토링에 따라오는 작업으로 Icon 컴포넌트를 개선하고 asset 몇 개를 추가하려고 했는데, 배포 결과물에서는 이 컴포넌트를 제공할 필요가 없더라고요. 인라인 svg로 가져오는 아이콘들에도 필요하지 않았어요.

  • 그래서 없애버려야겠다 생각했는데 Tabs에서도 얘를 쓰고있어서 ..ㅎㅎ 컴포넌트 개발에는 종종 사용될 수 있지 않을까 싶기도하고 어떻게 해야하나 고민 중이에요. 의견 주시면 좋겠어요 ~

📷 스크린샷

@se030 se030 added the ✨ feat 기능 구현 label Apr 22, 2023
@se030 se030 self-assigned this Apr 22, 2023
@se030 se030 marked this pull request as ready for review April 23, 2023 06:10
leesunmin1231
leesunmin1231 approved these changes Apr 23, 2023
View reviewed changes
Copy link
Contributor

@leesunmin1231 leesunmin1231 left a comment

Choose a reason for hiding this comment

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

Input에서도 Icon사용하고 있는데, 만약 deprecate된다면 leadingIcon을 ReactNode로 받는 방식으로 리팩토링 해야겠네욤.
사용방식은 크게 달라지지 않는거 같아서 저는 deprecate되도 상관 없을거 같아요~~

prayinforrain
prayinforrain approved these changes Apr 23, 2023
View reviewed changes
Copy link
Contributor

@prayinforrain prayinforrain left a comment

Choose a reason for hiding this comment

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

기존 Icon이 엄청 잘 만들었다고 생각했는데 react-icons를 래핑하는 느낌이라 위치가 좀 애매하긴 했죠. 어차피 바꾸긴 해야 했겠네요. 좋아요~

svg로 사용하지 않고 tsx로 만드신 건 커스터마이징 적용을 위해서인가요?

iyu88
iyu88 approved these changes Apr 23, 2023
View reviewed changes
Copy link
Member

@iyu88 iyu88 left a comment

Choose a reason for hiding this comment

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

제가 Icon 충성 고객이였군요

deprecated 된다면 저도 선민님처럼 ReactNode로 받는 방식을 고려해보겠습니다.

감사합니다!

@se030
Copy link
Contributor Author

se030 commented Apr 24, 2023

svg로 사용하지 않고 tsx로 만드신 건 커스터마이징 적용을 위해서인가요?

이건 확장자를 말씀하시는게 맞나요? 애셋을 추가 요청하지 않고 HTML 문서 내용에 포함시키기 위해서에요!

@se030 se030 merged commit 471e357 into main Apr 24, 2023
@se030 se030 deleted the feat/icon branch April 24, 2023 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iyu88 iyu88 iyu88 approved these changes

@prayinforrain prayinforrain prayinforrain approved these changes

@leesunmin1231 leesunmin1231 leesunmin1231 approved these changes

Assignees

@se030 se030

Labels
✨ feat 기능 구현
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feat: Icon 컴포넌트 개선
4 participants
@se030 @iyu88 @prayinforrain @leesunmin1231