Skip to content

Conversation

@Dogdriip
Copy link
Contributor

@Dogdriip Dogdriip commented Jul 12, 2022

Summary

해당 PR은 Radix 도입 및 Bezier와의 integration에 대한 PoC 정도로, 여기서 충분한 논의를 거쳐 적용 방식 등을 통일한 후 점진적으로 적용해 나가면 좋겠습니다.

See also: #788

  • Divider 컴포넌트에 withoutParallelIndent prop을 추가합니다.
  • Divider 컴포넌트에 Radix를 적용합니다.
  • ARIA Role 등에 기반한 단위 테스트를 추가합니다.

Details

변경점

  • BREAKING_CHANGE: Divider가 더 이상 HTMLHRElement가 아닌 HTMLDivElement입니다.
    • 기존 사용처에서 & > hr 등과 같이 태그 이름을 지정해 스타일링한 경우가 있다면 마이그레이션이 필요합니다.
  • withoutParallelIndent?: boolean = false prop을 추가합니다.
    • Divider와 수평 방향의 여백 (horizontal의 경우 상하 여백, vertical의 경우 좌우 여백)의 유무를 조절합니다.
    • 기존의 'Divider 양 끝'의 여백 유무를 조절하던 withoutSideIndent와 네이밍을 최대한 맞췄습니다.
  • withoutIndent?: boolean = false prop을 추가합니다.
    • Divider의 모든 방향 여백 유무를 조절합니다.
  • Radix를 이용해 Accessibility를 강화합니다.
    • decorative: boolean | undefined prop을 추가합니다.
      • 정말로 semantic하게 컨텐츠를 나누는 것인지, 단순한 graphical seperator인지를 나타냅니다. ARIA 속성의 적용 여부를 조절합니다.
    • 이외의 Radix 도입과 관한 이야기는 하술합니다.
  • Rounding에 round1을 추가합니다.

Radix 도입과 관한 이야기

  • Radix Primitive(이하 Radix)에서는 A11y 속성이 적용된 컴포넌트를 부분별로 제공합니다.
    • 이 PR은 Separator를 적용했습니다.
    • '부분별로 제공'한다는 말은 더 복잡한 컴포넌트에서 이해할 수 있습니다. Slider의 경우, Root, Track, Range, Thumb를 각각 제공합니다.
  • 관건은 '이를 Bezier와 어떻게 integrate할 것인가'입니다. 아래는 제가 시도했던 방법들과 각 방법에 대한 생각입니다.
    • ❌ Radix 컴포넌트에 그대로 bezier-reactstyled 등을 사용합니다.
      • Foundation 등이 적용되지 않습니다.
    • ⚠️ Radix 컴포넌트(Primitive)나 @radix-ui/react-polymorphic을 이용한 Polymorphic component를 extend하여 bezier를 주입합니다.
      • @radix-ui/react-polymorphic는 Deprecated되었으며, 아래 방법이 훨씬 간단하기 때문에 굳이 이렇게까지 할 필요는 없다고 생각했습니다.
    • asChild prop을 이용해 bezier 컴포넌트에 accessibility를 주입합니다.
      • Radix Primitive를 단순히 child 컴포넌트에 accessibility나 기타 functional requirements를 주입하는 용도로 사용할 수 있습니다. 미리 구현된 디자인 시스템 등에 Radix를 사용할 경우 대부분 이렇게 사용하는 듯합니다.
      • 다음은 Divider.tsx의 일부입니다.
        <SeparatorPrimitive.Root
          asChild
          orientation={orientation}
          decorative={decorative}
        >
          <Styled.Divider
            ref={forwardedRef}
            data-testid={testId}
            orientation={orientation}
            decorative={decorative}
            withoutSideIndent={withoutSideIndent}
            withoutParallelIndent={withoutParallelIndent}
            withoutIndent={withoutIndent}
            {...rest}
          />
        </SeparatorPrimitive.Root>

Browser Compatibility

OS / Engine 호환성을 반드시 확인해주세요.

Windows

  • Chrome - Blink
  • Edge - Blink
  • Firefox - Gecko (Option)

macOS

  • Chrome - Blink
  • Edge - Blink
  • Safari - WebKit
  • Firefox - Gecko (Option)

References

@Dogdriip Dogdriip added refactoring Issue or PR related to refactoring with no functional changes a11y Issue or PR related to accessibility labels Jul 12, 2022
@Dogdriip Dogdriip self-assigned this Jul 12, 2022
@changeset-bot
Copy link

changeset-bot bot commented Jul 12, 2022

🦋 Changeset detected

Latest commit: 486e8ed

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

@Dogdriip Dogdriip requested a review from sungik-choi July 12, 2022 13:47
@inhibitor1217 inhibitor1217 self-requested a review July 13, 2022 03:41
@Dogdriip Dogdriip marked this pull request as draft July 13, 2022 09:15
@Dogdriip Dogdriip changed the title Enhance Divider component with Radix add withoutParallelIndent prop and apply Radix to Divider component Jul 14, 2022
@codecov
Copy link

codecov bot commented Jul 14, 2022

Codecov Report

Merging #873 (486e8ed) into next-v1 (46caba3) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           next-v1     #873      +/-   ##
===========================================
+ Coverage    70.87%   71.00%   +0.13%     
===========================================
  Files          204      204              
  Lines         2884     2894      +10     
  Branches       797      807      +10     
===========================================
+ Hits          2044     2055      +11     
+ Misses         722      721       -1     
  Partials       118      118              
Impacted Files Coverage Δ
...ier-react/src/components/Divider/Divider.styled.ts 100.00% <100.00%> (ø)
...es/bezier-react/src/components/Divider/Divider.tsx 100.00% <100.00%> (+16.66%) ⬆️
...ages/bezier-react/src/foundation/Rounding/index.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Dogdriip Dogdriip marked this pull request as ready for review July 14, 2022 07:13
Copy link
Contributor

@sungik-choi sungik-choi left a comment

Choose a reason for hiding this comment

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

  • 윈도우 포함, 브라우저별로 잘 표시되는 지 확인부탁드려요!
  • 사방 마진을 제거하는 withoutIndent prop이 추가되면 사용처에서 더 쉽게 사용할 수 있을 거 같습니다. 아마 대부분 withoutSideIndent, withoutIndent 둘만 사용하게 될 거 같아요

@Dogdriip
Copy link
Contributor Author

  • 윈도우 포함, 브라우저별로 잘 표시되는 지 확인부탁드려요!
  • 사방 마진을 제거하는 withoutIndent prop이 추가되면 사용처에서 더 쉽게 사용할 수 있을 거 같습니다. 아마 대부분 withoutSideIndent, withoutIndent 둘만 사용하게 될 거 같아요
  • 윈도우는 집 가서 체크해보겠습니당 🏡
  • 지금 props에서 withoutIndent를 하나 더 추가하자는 말씀이 맞을까요?

@sungik-choi
Copy link
Contributor

sungik-choi commented Jul 15, 2022

  • 윈도우 포함, 브라우저별로 잘 표시되는 지 확인부탁드려요!
  • 사방 마진을 제거하는 withoutIndent prop이 추가되면 사용처에서 더 쉽게 사용할 수 있을 거 같습니다. 아마 대부분 withoutSideIndent, withoutIndent 둘만 사용하게 될 거 같아요
  • 윈도우는 집 가서 체크해보겠습니당 🏡
  • 지금 props에서 withoutIndent를 하나 더 추가하자는 말씀이 맞을까요?

네 추가하는 방향이 맞습니다! 테스트용 윈도우 노트북이 사내에 있으니, 크로마틱 배포된 스토리북으로 접속하셔서 확인하셔도 될거에요

@sungik-choi
Copy link
Contributor

Browser Compatibility만 체크되면 Approve 하겠습니다!

@Dogdriip
Copy link
Contributor Author

Browser Compatibility만 체크되면 Approve 하겠습니다!

최대한 체크해 보았는데 대부분의 브라우저에서 문제 없는 것 같습니다. PR 본문에 체크해두었으니 확인 부탁드립니다! 🙇

@github-actions
Copy link
Contributor

github-actions bot commented Jul 26, 2022

Chromatic Report

🚀 Congratulations! Your build was successful!

@Dogdriip Dogdriip merged commit 3b48467 into channel-io:next-v1 Sep 4, 2022
@Dogdriip Dogdriip deleted the feat/divider-radix-ui branch September 4, 2022 11:05
@sungik-choi sungik-choi added the #reimplementation Issue or PR related to Reimplementation of existing components (#1105) label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a11y Issue or PR related to accessibility refactoring Issue or PR related to refactoring with no functional changes #reimplementation Issue or PR related to Reimplementation of existing components (#1105)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants