channel-io / bezier-react

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

Migrate `FormHelperText`, `FormErrorMessage`, and `FormLabel` component with scss #1893

Closed yangwooseong closed 5 months ago

yangwooseong commented 5 months ago

Self Checklist

Related Issue

Summary

Details

Breaking change? (Yes/No)

References

changeset-bot[bot] commented 5 months ago

🦋 Changeset detected

Latest commit: 1a45cfe940b1c8f19a88cacfd3db47035b64464b

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

This PR includes changesets to release 1 package | Name | Type | | ------------------------ | ----- | | @channel.io/bezier-react | Major |

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!

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (1d6c349) 85.96% compared to head (1a45cfe) 85.94%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## alpha #1893 +/- ## ========================================== - Coverage 85.96% 85.94% -0.02% ========================================== Files 234 232 -2 Lines 3341 3337 -4 Branches 747 745 -2 ========================================== - Hits 2872 2868 -4 Misses 391 391 Partials 78 78 ```

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

sungik-choi commented 5 months ago

지금은 Text의 속성을 그대로 노출하고 있는데, 사실상 고정된 스타일(color, typo)를 사용하고 있기 때문에 이런 속성역시 제거해야되나 고민됩니다. 어떻게 생각하시는지 궁금합니다

사용처 확인해보고 오버라이드하는 곳이 거의 없다면 제거해도 좋을 거 같네요 👍

yangwooseong commented 5 months ago

FormLabel, FormHelperText가 Wrapper를 가지고 있지 않았기 때문에 스탠드얼론으로 사용할 수도 있었어요. Wrapper를 각 컴포넌트에서 가지고 있도록 변경되면, FormControl 의 레이아웃에 의존하는 padding, margin 속성들이 적용되어서 스탠드얼론으로 사용되는 케이스들의 레이아웃이 틀어지게 됩니다.

아하 그렇군요 제가 오해했네요. 그럼 이전 구현처럼 Control 에서 스타일링 된 Wrapper를 컨텍스트로 받도록 변경하겠습니다

yangwooseong commented 5 months ago

FormLabel, FormHelperText가 Wrapper를 가지고 있지 않았기 때문에 스탠드얼론으로 사용할 수도 있었어요. Wrapper를 각 컴포넌트에서 가지고 있도록 변경되면, FormControl 의 레이아웃에 의존하는 padding, margin 속성들이 적용되어서 스탠드얼론으로 사용되는 케이스들의 레이아웃이 틀어지게 됩니다.

아하 그렇군요 제가 오해했네요. 그럼 이전 구현처럼 Control 에서 스타일링 된 Wrapper를 컨텍스트로 받도록 변경하겠습니다

className 을 주는게 더 간단해 보여서 컨텍스트로 Wrapper가 아닌 className을 내려주도록 했습니다. FormControl과 같이 쓰이지 않는 경우에는 이전 구현처럼 div로 감싸지 않은 상태로 렌더링되도록 했습니다. 사실 굳이 하나 더 div element 를 주지 않고 className만 합치면 될 것 같은데, 고민하다가 이 부분은 이전과 동일하게 별도의 div element를 주는 방식으로 했습니다.