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

[DS-100] Button startIcon prop 활성화 #145

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

LTakhyunKim
Copy link
Contributor

@LTakhyunKim LTakhyunKim commented Nov 29, 2023

DS-100

기존 Button startIcon 을 활용할 수 없는 문제를 해결합니다.
디자인 시스템에서 start Icon 만 적용되므로, icon 이라는 새로운 prop 을 추가 후 반영했으나
end Icon 은 사용 가능하지만, start Icon 이 적용이 안되는 건 이상하다고 생각하여 다시 살리는 방향으로 수정했습니다.

이미 디자인 시스템을 사용 중인 프로젝트가 있고, icon prop 을 활용하고 있어 icon prop 은 삭제하지 않았습니다.
차후 deprecated -> icon prop 삭제 방향으로 생각하고 있는데 이에 대한 의견을 듣고 싶습니다.

@LTakhyunKim LTakhyunKim added the bug Something isn't working label Nov 29, 2023
@LTakhyunKim LTakhyunKim requested a review from deminoth November 29, 2023 06:49
@LTakhyunKim LTakhyunKim self-assigned this Nov 29, 2023
Copy link

github-actions bot commented Nov 29, 2023

@deminoth
Copy link
Member

end Icon 은 사용 가능하지만, start Icon 이 적용이 안되는 건 이상하다고 생각하여 다시 살리는 방향으로 수정했습니다.

제가 처음 이슈 확인했을때 추측한 것은, 디자인시스템 상으로 endIcon이 불가능하기 때문에 icon이라는 prop만 제공한게 아닌가 였습니다. 실제로 피그마상으로도 그런 예시가 없고, Attributes 목록에도 Left icon만 있네요. 그렇다면 startIcon과 endIcon을 둘 다 사용할 수 없는 방향으로 수정이 맞지 않을까요?

@LTakhyunKim
Copy link
Contributor Author

제가 처음 이슈 확인했을때 추측한 것은, 디자인시스템 상으로 endIcon이 불가능하기 때문에 icon이라는 prop만 제공한게 아닌가 였습니다.

말씀해주신 내용은 맞습니다.
다만 저는 둘 다 사용은 할 수 있게 살리는 방향을 선택했었는데 오히려 없애는 방향이 좋다고 생각하셨군요. 🤔

endIcon 의 경우, 디자인 시스템에서도 활용하지 않기에 살려도 큰 문제가 없다고 생각했습니다.
디자인 시스템에서는 left icon 즉, start Icon 만 활용하기에 start Icon prop 도 살려서 이를 활용하는 방향으로
가는 것도 Mui API 통일성 면에서 좋다고 생각했습니다.

다만 이는 개인적인 의견이라 삭제가 좋은 방향이라면 그대로 진행하겠습니다!

@deminoth
Copy link
Member

넵 startIcon, endIcon은 동작하기만 한다면 살려둬도 괜찮을 것 같습니다!

다만 저희 작업 원칙이 figma와 가능한 일치시키는 것이니, icon은 deprecate 시키지 않고 startIcon과 동일한 동작 하도록 남겨둬야겠습니다.
스크린샷 2023-11-29 오후 4 24 16

@LTakhyunKim
Copy link
Contributor Author

넵 startIcon, endIcon은 동작하기만 한다면 살려둬도 괜찮을 것 같습니다!

현재 startIcon, endIcon 동작하고 있습니다!

다만 저희 작업 원칙이 figma와 가능한 일치시키는 것이니, icon은 deprecate 시키지 않고 startIcon과 동일한 동작 하도록 남겨둬야겠습니다.

말씀해주신 내용처럼 icon 은 유지하겠습니다 코멘트 감사합니다. 👍

@LTakhyunKim LTakhyunKim merged commit 6179286 into main Nov 29, 2023
5 checks passed
@LTakhyunKim LTakhyunKim deleted the ds-100-button-startIcon-prop branch November 29, 2023 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants