channel-io / bezier-react

React components library that implements Bezier design system.
https://main--62bead1508281287d3c94d25.chromatic.com
Apache License 2.0
181 stars 45 forks source link

Implement Box layout component #1785

Closed sungik-choi closed 5 months ago

sungik-choi commented 5 months ago

Self Checklist

Related Issue

Summary

Implement Box layout component

Details

자세한 내용은 이슈를 참고해주세요. 구현하며 아래와 같은 점들을 신경썼습니다.

Breaking change? (Yes/No)

No

References

changeset-bot[bot] commented 5 months ago

🦋 Changeset detected

Latest commit: 99f517f60dea923e8ec5ccd6252ae6b484ebc483

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages | Name | Type | | ------------------------ | ----- | | @channel.io/bezier-react | Minor | | bezier-figma-plugin | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions[bot] commented 5 months ago

Chromatic Report

🚀 Congratulations! Your build was successful!

sungik-choi commented 5 months ago

https://github.com/radix-ui/themes/blob/main/packages/radix-ui-themes/src/helpers/props/margin.props.ts 과 같은 식으로 helper function을 가까이에 위치시키자. Form related prop도 함께 적용해볼 수 있을듯.

codecov[bot] commented 5 months ago

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (3e1ecae) 87.12% compared to head (99f517f) 86.64%. Report is 1 commits behind head on alpha.

Files Patch % Lines
packages/bezier-react/src/components/Box/Box.tsx 0.00% 8 Missing :warning:
packages/bezier-react/src/utils/props.ts 53.33% 7 Missing :warning:
packages/bezier-react/src/utils/style.ts 50.00% 2 Missing and 1 partial :warning:
...r-react/src/components/ListItem/ListItem.styled.ts 0.00% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## alpha #1785 +/- ## ========================================== - Coverage 87.12% 86.64% -0.48% ========================================== Files 277 278 +1 Lines 3798 3790 -8 Branches 794 796 +2 ========================================== - Hits 3309 3284 -25 - Misses 416 433 +17 Partials 73 73 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sungik-choi commented 5 months ago

기존 컴포넌트들과 다르게 width, height 등 dimension property에 number를 넣으면 px를 붙여주는 로직(e.g. ModalContent)을 제외했습니다. primitive layout 컴포넌트라는 특성상, 애플리케이션에서 굉장히 많이 호출될 수 있습니다. 꼭 필요한 것이 아니라면 number 체크 로직을 제거하고자 했습니다.

https://github.com/facebook/react/blob/8b8d265bd9a4cab7bbd04a9a13950fdc946ea51c/packages/react-dom-bindings/src/client/CSSPropertyOperations.js#L67-L101

React에서도 value === number type && !== 0 && !== unitless number property 일 경우에 암시적으로 px 을 붙여주고 있으므로, React와 동작을 통일하는 편이 React에 익숙한 사용자가 라이브러리를 별도의 학습 비용 없이 쉽게 사용할 수 있을 거 같습니다. 약간의 오버헤드를 고려하더라도 추가하는 게 나을 거 같네요.

yangwooseong commented 5 months ago

기존 컴포넌트들과 다르게 width, height 등 dimension property에 number를 넣으면 px를 붙여주는 로직(e.g. ModalContent)을 제외했습니다. primitive layout 컴포넌트라는 특성상, 애플리케이션에서 굉장히 많이 호출될 수 있습니다. 꼭 필요한 것이 아니라면 number 체크 로직을 제거하고자 했습니다.

https://github.com/facebook/react/blob/8b8d265bd9a4cab7bbd04a9a13950fdc946ea51c/packages/react-dom-bindings/src/client/CSSPropertyOperations.js#L67-L101

React에서도 value === number type && !== 0 && !== unitless number property 일 경우에 암시적으로 px 을 붙여주고 있으므로, React와 동작을 통일하는 편이 React에 익숙한 사용자가 라이브러리를 별도의 학습 비용 없이 쉽게 사용할 수 있을 거 같습니다. 약간의 오버헤드를 고려하더라도 추가하는 게 나을 거 같네요.

저도 오버헤드가 그리 크지는 않을 것 같아서 추가해도 될 것 같아요