Migrate `Divider` component with scss #1861

yangwooseong · 2023-12-26T05:25:48Z

Self Checklist

I wrote a PR title in English and added an appropriate label to the PR.
I wrote the commit message in English and to follow the Conventional Commits specification.
I added the changeset about the changes that needed to be released. (or didn't have to)
I wrote or updated documentation related to the changes. (or didn't have to)
I wrote or updated tests related to the changes. (or didn't have to)
I tested the changes in various browsers. (or didn't have to)
- Windows: Chrome, Edge, (Optional) Firefox
- macOS: Chrome, Edge, Safari, (Optional) Firefox

Related Issue

Migrating component styling to Sass-based #1733

Summary

Divider 컴포넌트의 스타일링을 styled-components 에서 scss 으로 변경합니다.
기존의 조건부 스타일링을 scss기반의 nested한 형태로 구현합니다

Details

Divider 컴포넌트의 인터페이스에 MarginProps가 추가됩니다.

Breaking change? (Yes/No)

No

References

None

- it had been exported from styled component

changeset-bot · 2023-12-26T05:25:51Z

⚠️ No Changeset found

Latest commit: 431bea4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

github-actions · 2023-12-26T05:28:27Z

Chromatic Report

🚀 Congratulations! Your build was successful!

codecov · 2023-12-26T05:32:03Z

Codecov Report

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

Comparison is base (062f8a8) 86.37% compared to head (431bea4) 86.27%.

Files	Patch %	Lines
...es/bezier-react/src/components/Divider/Divider.tsx	84.61%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha    #1861      +/-   ##
==========================================
- Coverage   86.37%   86.27%   -0.11%     
==========================================
  Files         275      274       -1     
  Lines        3765     3751      -14     
  Branches      795      789       -6     
==========================================
- Hits         3252     3236      -16     
  Misses        434      434              
- Partials       79       81       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yangwooseong · 2023-12-26T05:46:47Z

packages/bezier-react/src/components/Divider/Divider.test.tsx

-        })
-        const divider = getByTestId(DIVIDER_TEST_ID)
-
-        expect(divider).toHaveStyle('margin: 6px')


{ orientation: 'horizontal' } 속성을 주었을 때 horizontal classname 을 갖는 것을 테스트하는 것은 무의미하다고 생각하여 지웠습니다

yangwooseong · 2023-12-26T05:47:43Z

packages/bezier-react/src/components/Divider/index.ts

@@ -1,3 +1,2 @@
-export { default as Divider } from './Divider'
+export { Divider, DIVIDER_THICKNESS } from './Divider'


SegmentedControlIndicator 에서 사용됩니다

DIVIDER_THICKNESS 의 중복을 줄이면 더 좋을 거 같은데요, 어떤 방법이 좋을지 고민입니다.

scss export를 써서(export:) scss에서 DIVIDER_THICKNESS를 export한다...

$DIVIDER_THICKNESS: 1; :export { DIVIDER_THICKNESS: $DIVIDER_THICKNESS; }

styles/ 디렉터리에 전역 components layer의 css variable을 만든다... 이 편이 더 CSS스럽고 특정 기술에 덜 의존적이라 1번보다는 마음에 듭니다.

@layer components { --b-divider-thickness: 1px; }

둘다 좋아보이지만 고르라면 저도 2번 인 것 같습니다!

-> 다시 생각해보니 2번으로 하면 divider 관련 상수가 divider 로부터 멀리 떨어지는 것이 아쉬워지네요. 그리고 이렇게 selector 없이 css variable 만 선언할 수 있나요? :where(:root) 같은 selector 를 선언하기 애매한 부분도 있는 것 같아요

이렇게 selector 없이 css variable 만 선언할 수 있나요? -> 예시를 잘못 작성했어요. :where(:root) 같은 selector 필요합니다..!

전역으로 이동하는 게 애매하다면... 이런 식으로 해볼 수도 있을 거 같아요

.Divider { .variables { --b-divider-thickness: 1px; } /* ... */ }

import dividerStyles from '../../Divider.module.scss' divdierStyles.variables

이렇게 해서 dividerStyles.variables를 가져와도 className이라서 css variable를 가져오는 것과는 다른 것 같습니다. 아마 js에서 css 에서 선언된 변수를 가져오기는 어려운 걸로 알고 있어요.

그래서 divider.tsx 에서 DIVIDER_THICKNESS를 css variable(--b-divider-thickness)로서 넘기고, 다른 컴포넌트에서 사용한다면 DIVIDER_THICKNESS를, 다른 css 파일에서 사용한다면 --b-divider-thickness를 import 해서 쓰게 하면 될 것 같네요 (431bea4)

제가 컨텍스트를 하나 건너뛰었네요. 이 부분이 적용되려면 SegmentedControlIndicator 의 스타일링 방식도 JS 변수 -> 인라인 스타일 주입에서 CSS 내부에서 css variable을 사용해서 calc하는 방식으로 변경되어야할 거 같아요.

이건 제가 후속 PR로 SegmentedControl scss 마이그레이션하면서 css variable만 사용할 수 있도록 리팩토링해볼게요

sungik-choi · 2023-12-26T09:46:03Z

packages/bezier-react/src/components/Divider/Divider.module.scss

+
+      &.without-indent {
+        width: 100%;
+        margin: 0;


Divider가 가지고 있는 margin 속성의 우선순위때문에(nested selector) 공통 margin prop이 적용되지 않는 문제가 있습니다.

더 나아가서 margin prop을 지원하지 않는게 좋다고 생각해요. 저희 디자인 시스템의 Divider 컴포넌트는 특이하게 컴포넌트 외부의 margin 또한 컴포넌트 스펙의 일부여서(DIVIDER_INDENT_SIZE 6px) 이 값을 커스텀할 수 있게 하면 안될 거 같아요..! 어떻게 생각하시는지 궁금합니다.

아하 그렇네요..! 지원안하는 것으로 변경하겠습니다

sungik-choi · 2023-12-26T10:02:38Z

packages/bezier-react/src/components/Divider/index.ts

@@ -1,3 +1,2 @@
-export { default as Divider } from './Divider'
+export { Divider, DIVIDER_THICKNESS } from './Divider'


DIVIDER_THICKNESS 의 중복을 줄이면 더 좋을 거 같은데요, 어떤 방법이 좋을지 고민입니다.

scss export를 써서(export:) scss에서 DIVIDER_THICKNESS를 export한다...

$DIVIDER_THICKNESS: 1; :export { DIVIDER_THICKNESS: $DIVIDER_THICKNESS; }

styles/ 디렉터리에 전역 components layer의 css variable을 만든다... 이 편이 더 CSS스럽고 특정 기술에 덜 의존적이라 1번보다는 마음에 듭니다.

@layer components { --b-divider-thickness: 1px; }

sungik-choi · 2023-12-26T10:04:44Z

packages/bezier-react/src/components/Divider/Divider.types.ts


-interface DividerOptions {
+interface DividerOwnProps {


약간 변덕스럽게 **Options interface를 더 잘 어울리는 거 같다는 이유로 조용히 **OwnProps로 변경했었는데... 적용해주셔서 감사합니다 ㅎㅎ

yangwooseong added 6 commits December 26, 2023 14:02

feat(divider): redefine DividerProps

b48fb93

feat(divider): change divider component to be styled with scss

c830fac

feat(divider): remove unnecessary style test

d058f3e

refactor(divider): change to named export

fff468b

feat(divider): export DIVIDER_THINKNESS from component

765109e

- it had been exported from styled component

chore(divider): add changeset

9a06f8d

yangwooseong self-assigned this Dec 26, 2023

docs(divider): add JSDoc

73f5aa8

refactor(divider): use satisfies in story

94e1823

yangwooseong changed the title ~~Feat/sass/divider~~ Migrate Divider component with scss Dec 26, 2023

yangwooseong commented Dec 26, 2023

View reviewed changes

yangwooseong marked this pull request as ready for review December 26, 2023 05:48

yangwooseong requested a review from sungik-choi as a code owner December 26, 2023 05:48

sungik-choi reviewed Dec 26, 2023

View reviewed changes

feat(divider): remove margin related props and changeset

e8a2034

yangwooseong added the enhancement Issues or PR related to making existing features better label Dec 27, 2023

refactor(divider): use css variable instead of scss variable

431bea4

yangwooseong requested a review from sungik-choi December 28, 2023 05:12

sungik-choi approved these changes Dec 28, 2023

View reviewed changes

yangwooseong merged commit e91c4aa into channel-io:alpha Dec 28, 2023
4 of 6 checks passed

yangwooseong deleted the feat/sass/divider branch December 28, 2023 05:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `Divider` component with scss #1861

Migrate `Divider` component with scss #1861

yangwooseong commented Dec 26, 2023 •

edited

Loading

changeset-bot bot commented Dec 26, 2023 •

edited

Loading

github-actions bot commented Dec 26, 2023 •

edited

Loading

codecov bot commented Dec 26, 2023 •

edited

Loading

yangwooseong Dec 26, 2023

yangwooseong Dec 26, 2023

sungik-choi Dec 26, 2023

yangwooseong Dec 27, 2023

sungik-choi Dec 28, 2023

yangwooseong Dec 28, 2023

sungik-choi Dec 28, 2023 •

edited

Loading

sungik-choi Dec 26, 2023

yangwooseong Dec 27, 2023

sungik-choi Dec 26, 2023

sungik-choi Dec 26, 2023

		@@ -1,3 +1,2 @@
		export { default as Divider } from './Divider'
		export { Divider, DIVIDER_THICKNESS } from './Divider'


		interface DividerOptions {
		interface DividerOwnProps {

Migrate Divider component with scss #1861

Migrate Divider component with scss #1861

Conversation

yangwooseong commented Dec 26, 2023 • edited Loading

Self Checklist

Related Issue

Summary

Details

Breaking change? (Yes/No)

References

changeset-bot bot commented Dec 26, 2023 • edited Loading

⚠️ No Changeset found

github-actions bot commented Dec 26, 2023 • edited Loading

Chromatic Report

codecov bot commented Dec 26, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sungik-choi Dec 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Migrate `Divider` component with scss #1861

Migrate `Divider` component with scss #1861

yangwooseong commented Dec 26, 2023 •

edited

Loading

changeset-bot bot commented Dec 26, 2023 •

edited

Loading

github-actions bot commented Dec 26, 2023 •

edited

Loading

codecov bot commented Dec 26, 2023 •

edited

Loading

sungik-choi Dec 28, 2023 •

edited

Loading