search

channel-io / bezier-react

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

implement AlphaBezierProvider #1303

Closed dinohan closed 1 year ago

dinohan commented 1 year ago

Self Checklist

Related Issue

Fixes #1301

Summary

BezierProvider를 대체할 수 있는 AlphaBezierProvider를 추가합니다.

Details

1301 에 자세한 문제 상황을 서술하였습니다. 요약하자면,

Breaking change or not (Yes/No)

References

changeset-bot[bot] commented 1 year ago

🦋 Changeset detected

Latest commit: b1363af09812aba82816c10dd9c16d4dacf1fedd

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 1 year ago

Chromatic Report

🚀 Congratulations! Your build was successful!

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 :tada:

Comparison is base (542a624) 78.18% compared to head (b1363af) 78.21%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## next-v1 #1303 +/- ## =========================================== + Coverage 78.18% 78.21% +0.02% =========================================== Files 294 295 +1 Lines 3796 3800 +4 Branches 838 838 =========================================== + Hits 2968 2972 +4 Misses 545 545 Partials 283 283 ``` | [Impacted Files](https://codecov.io/gh/channel-io/bezier-react/pull/1303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io) | Coverage Δ | | |---|---|---| | [...bezier-react/src/providers/AlphaBezierProvider.tsx](https://codecov.io/gh/channel-io/bezier-react/pull/1303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io#diff-cGFja2FnZXMvYmV6aWVyLXJlYWN0L3NyYy9wcm92aWRlcnMvQWxwaGFCZXppZXJQcm92aWRlci50c3g=) | `100.00% <100.00%> (ø)` | | | [...ages/bezier-react/src/providers/BezierProvider.tsx](https://codecov.io/gh/channel-io/bezier-react/pull/1303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io#diff-cGFja2FnZXMvYmV6aWVyLXJlYWN0L3NyYy9wcm92aWRlcnMvQmV6aWVyUHJvdmlkZXIudHN4) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

junbong commented 1 year ago

'Alpha'라는 접두어가 이름만 봐서는 의미를 알아채기 어려워 보이긴 하는데 Lite 이런건 어떨까요?

dinohan commented 1 year ago

'Alpha'라는 접두어가 이름만 봐서는 의미를 알아채기 어려워 보이긴 하는데 Lite 이런건 어떨까요?

새 Provider 이름을 어떻게 지을까 고민하다가 Alpha- 라는 prefix를 쓰는 다른 작업들을 참고하여 작성했습니다.

기존의 BezierComponent는 장기적으로는 deprecate되어야 할 것 같습니다. AlphaBezierProvider가 진짜 BezierProvider가 되고, EnableCSSHoudini는 별도로 import해서 실행하는 형태가 최종 목표입니다.

기존의 레거시 컴포넌트를 대체하는데, breaking change가 있는 컴포넌트 네이밍에 Alpha-를 쓰는 것 같아 따르기로 했습니다.

처음 이런 네이밍을 사용하신 게 @sungik-choi 신데, 처음 AlphaCenter를 추가하실 때 의도가 이와 같은지 확인 부탁드립니다.

dinohan commented 1 year ago

생각해보니 장기적으로 기존 BezierProvider를 없애고자 한다면 이 PR에서 추가 작업이 필요하겠네요!

sungik-choi commented 1 year ago

처음 이런 네이밍을 사용하신 게 @sungik-choi 신데, 처음 AlphaCenter를 추가하실 때 의도가 이와 같은지 확인 부탁드립니다.

'기존의 레거시 컴포넌트를 대체하는데, breaking change가 있는 컴포넌트 네이밍에 Alpha-를 쓰는 것 같아 따르기로 했습니다.' 는 아닙니다. 기존 컴포넌트 대체 여부와는 상관 없이, 불안정하지만 미리 사용성을 테스트해보고 싶은 컴포넌트의 네이밍 앞에 Alpha** 를 붙이는 방식을 시도해보고 있어요. 방식은 Shopify Polaris를 참고했습니다.

breaking change가 있더라도 새로운 컴포넌트가 레거시 컴포넌트를 완벽히 대체할 수 있다고 판단하면 모두 대체하고, 극소수의 엣지 케이스를 커버할 수 없다고 판단하면 레거시 컴포넌트를 남겨두는 방식으로 작업하고 있어요.

sungik-choi commented 1 year ago 
<BezierExperimentalProvider smoothCorners> // smoothCorners={{ Avatar: true }} ?
  <BezierProvider foundation={foundation}>
    <App>
      <BezierProvider foundation={LightFoundation}>
        <Inner />
      </BezierProvider>
    </App>
  </BezierProvider>
</BezierExperimentalProvider>

이런 식의 설계는 어떨까요?

이렇게 변경할 경우 enableSmoothCorners, EnableCSSHoudini 를 React lifecycle 안에 포함시켜야하며 Avatar의 styled-components에 prop으로 기능 On/Off 플래그를 전달해야 합니다(현재는 .styled에서 직접 모듈을 import해서 참조)

더 나아가서 'paintWorklet' in CSS 조건을 충족했을 경우에만 lazy import 할 수도 있을 거 같은데, 이건 빌드 설정과도 연관이 깊어서 리서치가 필요할 거 같아요.

dinohan commented 1 year ago

이런 식의 설계는 어떨까요? ...

BezierExperimentalProvider은 smooth corner 뿐만 아니라 다른 실험적인 기능을 끄고 켜는 역할을 하는 Provider 같습니다. 아마 Context와 함께 const { smoothCorner } = useExperimental() 같은 hook이 동반되겠군요. 그렇다면 prop을 boolean에서 더 확장해서 아래와 같이 사용하는 것이 괜찮을 것 같습니다.

const smoothCorners = new SmoothCorners()

<BezierExperimentalProvider
  smoothCorners={smoothCorners}
>
  <BezierProvider foundation={foundation}>
    <App>
      <BezierProvider foundation={LightFoundation}>
        <Inner />
      </BezierProvider>
    </App>
  </BezierProvider>
</BezierExperimentalProvider>

이렇게 하면 나중에 새로운 실험적 기능 B가 추가 되었을 때 smoothCorner는 import하지 않고 B만 import 하는 등, 기능 확장 시 treeshaking에 더 유리할 것 같습니다.

sungik-choi commented 1 year ago

https://github.com/channel-io/bezier-react/pull/1309