Remove the CSS-in-JS library #1106
Assignees
Labels
epic
Issue consisting of multiple issues of the same purpose
priority:A
Issue that important and urgent
Comments
sungik-choi
added
the
epic
9 tasks
9 tasks
This was referenced Jul 11, 2023
Merged
sungik-choi
added a commit
that referenced
this issue
Dec 15, 2023
<!-- How to write a good PR title: - Follow [the Conventional Commits specification](https://www.conventionalcommits.org/en/v1.0.0/). - Give as much context as necessary and as little as possible - Prefix it with [WIP] while it’s a work in progress --> ## Self Checklist - [x] I wrote a PR title in **English** and added an appropriate **label** to the PR. - [x] I wrote the commit message in **English** and to follow [**the Conventional Commits specification**](https://www.conventionalcommits.org/en/v1.0.0/). - [x] I [added the **changeset**](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md) about the changes that needed to be released. (or didn't have to) - [x] I wrote or updated **documentation** related to the changes. (or didn't have to) - [x] I wrote or updated **tests** related to the changes. (or didn't have to) - [x] 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 <!-- Please link to issue if one exists --> - #1106 ## Summary <!-- Please brief explanation of the changes made --> Migrate `DisabledOpacity` constant to CSS variable ## Details <!-- Please elaborate description of the changes --> `DiabledOpacity` 상수를 `--opacity-disabled` CSS variable로 변경합니다. ### Breaking change? (Yes/No) <!-- If Yes, please describe the impact and migration path for users --> No ## References <!-- Please list any other resources or points the reviewer should be aware of --> - #1106
Closed
6 tasks
sungik-choi
added a commit
that referenced
this issue
Jan 11, 2024
…as` prop (#1899) <!-- How to write a good PR title: - Follow [the Conventional Commits specification](https://www.conventionalcommits.org/en/v1.0.0/). - Give as much context as necessary and as little as possible - Prefix it with [WIP] while it’s a work in progress --> ## Self Checklist - [x] I wrote a PR title in **English** and added an appropriate **label** to the PR. - [x] I wrote the commit message in **English** and to follow [**the Conventional Commits specification**](https://www.conventionalcommits.org/en/v1.0.0/). - [x] I [added the **changeset**](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md) about the changes that needed to be released. (or didn't have to) - [x] I wrote or updated **documentation** related to the changes. (or didn't have to) - [x] I wrote or updated **tests** related to the changes. (or didn't have to) - [x] 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 <!-- Please link to issue if one exists --> - #1106 ## Summary <!-- Please brief explanation of the changes made --> Change the `as` prop to allow all element types and button to allow `as` prop ## Details <!-- Please elaborate description of the changes --> - as 속성이 모든 엘리먼트 타입을 지원하도록 변경합니다. 단순 HTML element type 외에도, Link 컴포넌트를 as 속성에 주입하는 등의 케이스가 있습니다. 이를 지원하여 breaking change를 줄이기 위한 변경입니다. - `Button` 또한 마찬가지로 Link, a 태그 등으로 사용되는 케이스들이 있습니다. as 속성을 지원하도록 개선했습니다. ### Breaking change? (Yes/No) <!-- If Yes, please describe the impact and migration path for users --> No
sungik-choi
added a commit
that referenced
this issue
Feb 7, 2024
<!-- How to write a good PR title: - Follow [the Conventional Commits specification](https://www.conventionalcommits.org/en/v1.0.0/). - Give as much context as necessary and as little as possible - Prefix it with [WIP] while it’s a work in progress --> ## Self Checklist - [x] I wrote a PR title in **English** and added an appropriate **label** to the PR. - [x] I wrote the commit message in **English** and to follow [**the Conventional Commits specification**](https://www.conventionalcommits.org/en/v1.0.0/). - [x] I [added the **changeset**](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md) about the changes that needed to be released. (or didn't have to) - [x] I wrote or updated **documentation** related to the changes. (or didn't have to) - [x] I wrote or updated **tests** related to the changes. (or didn't have to) - [x] 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 <!-- Please link to issue if one exists --> - Fixes #1976 ## Summary <!-- Please brief explanation of the changes made --> Modify styles globally ## Details <!-- Please elaborate description of the changes --> ### Bug Fix - #1976 및 Avatar + Status의 gap이 제대로 적용되지 않던 문제를 수정합니다. - FormControl의 label, helperText className 내부 :where 사용으로, 우선순위가 낮아 스타일이 의도대로 적용되지 않는 문제를 수정합니다. - Spinner에 shortcut style이 적용되어 제대로 보이지 않던 문제를 수정합니다. ### Remove `@reset` layer & reset css reset css를 제거했습니다. reset css를 추가하기 이전에 모든 애플리케이션의 reset 스타일을 먼저 파악하고, 어느 범위까지 reset css를 적용할지 팀 단위에서 논의하는 게 우선되어야합니다. 이는 상당한 시간이 필요합니다. 일정 + 마이그레이션의 취지를 고려하자면 최대한 애플리케이션의 전역 상태에는 영향을 끼치지 않는 쪽으로 구현하는 게 이번 버전에서는 맞는 방향이라고 판단했습니다. reset css와 별개로 base.scss의 스타일을 애플리케이션에 선택 여부 없이 필수적으로 적용되어야하는 스타일이라고 판단하여 그대로 적용합니다. (마이그레이션 이전과 동일합니다) reset css에 의존하고 있었던 스타일을 독립적으로도 잘 작동하도록 변경합니다. - box-sizing: border-box 스타일이 padding/border를 가진 모든 요소에 적용되도록 합니다. - 불필요한 all: unset 스타일을 제거합니다. 대신 필요한 속성에 대해서만 초기화를 적용합니다 (normalize) - all: unset의 대부분은 HTML Button의 스타일을 초기화하기위한 코드였습니다. 스타일 초기화 + 커서 + 포커스 링이 적용된 `BaseButton` 컴포넌트를 만들고 이를 재사용했습니다. - BaseButton을 적용하며 `type="button"` 이 누락되어 있었던 사용처들에 button이 잘 적용됩니다. - Banner의 링크 태그에 스타일이 적용되지 않는 문제를 수정합니다. - the-new-css-reset 패키지를 제거합니다. ### Other enhancements - Tag의 삭제 아이콘이 키보드 포커싱 가능하도록 변경하고, 포커스 링 스타일을 적용했습니다. ### Miscellaneous 🤔 FYI. @yangwooseong - 지금 당장 해결할 문제는 아니지만, 작업하다보니 CSS Modules가 가지는 스타일 중복에 대한 문제가 느껴집니다. 사실 용량을 신경쓰지 않는다면 문제될 부분은 없지만, 신경쓰지 않을 수가 없는 문제같아요. SCSS 작업을 완료한 이후 CSS 최종 사이즈가 압축 이전 100KB 정도가 되니 마냥 작은 사이즈는 아닙니다. - 중복을 제거하자면 atomic class를 사용해야할텐데, 이번 마이그레이션 과정에서도 atomic class 비슷하게 구현한 부분들이 있으나(e.g. elevation.module.scss) 이를 다른 컴포넌트에서 사용하는 경험이 좋지 않고(모듈 import 후, className에 적용해야함) 스타일링 방식이 일관적이지 않아서 전역적으로 사용하지 않았어요. - atomic class를 구현하는 로직에서도 token 값을 재선언해야하는 문제가 있어요. token 데이터를 기반으로 atomic class를 바로 생성해주는 게 가장 좋아보이는데 말이죠. 예를 들어 아래와 같은 부분입니다. ```scss $radiuses: 2, 3, 4, 6, 8, 12, 16, 20, 32, 44, 42-p; // <-- 토큰 값이 변경되면 수동으로 변경해줘야해요 @each $radius in $radiuses { :where(.radius-#{$radius}) { border-radius: var(--radius-#{$radius}); } } ``` - #1106 에서 CSS modules를 선택했던 이유가 외부 의존성을 제거하기 위해서였기에 기대치를 충분히 잘 충족하는 선택이었다고 생각합니다. 하지만 더 나아가서 추후 css 사이즈 등을 더 신경써야한다면 atomic class 기반의 스타일 시스템(e.g. tailwind, panda css, stylex)을 **내부적으로만** 사용하는 걸 고려해봐야할 수도 있을 거 같아요. - 다행히 이번 마이그레이션을 통해 특정 라이브러리의 의존성을 제거했기 때문에, 언제가 될진 모르겠지만 그 때가 온다면, 그 때는 Breaking change 없이 점진적으로 마이그레이션을 잘 진행할 수 있을 거 같아요. ### Breaking change? (Yes/No) <!-- If Yes, please describe the impact and migration path for users --> No ## References <!-- Please list any other resources or points the reviewer should be aware of --> Reset CSS를 포함해야하는지, 포함한다면 어디까지 포함해야할까? - CMSgov/design-system#2116 - CMSgov/design-system#2333
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
styled-components라이브러리를 제거합니다. 대체할만한 스타일링 시스템을 리서치하고, 디자인 시스템 컴포넌트를 마이그레이션합니다.Reasons for suggestion
Why
#764 의 결정과 다르게 몇가지 주요한 이유로 styled-components를 제거하기로 결정했습니다.
1. SSR 환경에서 hash collision 문제
베지어 디자인 시스템이 사내 프로덕트에 광범위하게 사용됨에따라, SSR 환경 지원에 대한 필요성이 생겼습니다. 하지만 위와 같은 문제로 Next.js를 사용하는 프로젝트에서 디자인 시스템을 사용하지 못하고 있는 상황입니다. 다른 이슈 케이스들을 리서치해봤을 때 빠른 시일내로 해결되기 어려워보이고, 비슷한 문제가 이후에도 재발하지 않을 거란 보장이 없습니다. (추가: CSS-in-JS React Server Component에서 현재(23/10/13) 기준 적합하지 않음)
2. CSS Variable과 styled-components의 ThemeProvider(foundation) 혼용
라이브러리 사용처에서 styled-component를 사용하지 않는 경우, foundation 접근 없이도 색상값을 사용할 수 있도록 Theme 값들에 대해 CSS Variable을 추가했습니다. 둘 모두 사용가능하기에 현재 디자인 시스템/채널 어플리케이션 모두 두 가지 사용방식이 혼용되고 있는 상황입니다.
Theme 값 외 다른 속성값들도 CSS Variable로 사용할 수 있고, 더 나아가선 ThemeProvider가 없이 CSS Variable만으로 다이나믹 테마를 구현할 수 있습니다. 런타임 오버헤드(객체 접근) 없이도 동일한 기능을 구현할 수 있는 CSS Variable의 방식이 더 좋다고 판단했습니다. IE11 지원이 공식적으로 종료되었기 때문에, 이제 CSS Variable을 사용하더라도 지원 브라우저 범위를 모두 커버할 수 있게 되었습니다. CSS로 잘 할 수 있는 일을 JS를 통해서 할 필요는 없다고 판단했습니다.
3. styled-components를 오버라이드하면서 생기는 유지보수 문제
theme속성밖에 사용할 수 없는ThemeProvider에foundation속성을 사용하기 위해 styled-components를 오버라이드하여 사용하고 있습니다. 그 과정에서 엔지니어가 잊고 내보내지 못한 모듈도 생기고, 트랜스파일링 관련 이슈도 발생했습니다. 현재 어느정도 정리가 된 상황이나, 추후 라이브러리에 Breaking change가 발생한다면 동일한 문제가 계속해서 발생할 확률이 굉장히 높습니다.그 외 항상 언급되는 성능 관련 문제도 있지만, 마이그레이션의 주된 이유는 아닙니다. 하지만 어플리케이션의 사이즈가 점점 커지면서 런타임 오버헤드가 무시할 수 없을 거라 '예상'됩니다. 이는 전후 비교 퍼포먼스 측정을 통해서 알아보면 좋겠습니다.
Proposed Solution
CSS-in-JS를 제거한 다음 버전의 적절한 스타일 시스템은 Sass(CSS) Modules을 선택하고자 합니다. '왜 Tailwind CSS를 쓰지 않죠?' 와 같은 의문이 들 수 있을 거라 생각합니다. 아래와 같은 고민들을 통해 결정했습니다.
Hurdles
마이그레이션 과정에 몇 가지 허들이 있으며, 이 허들을 넘기 위한 방법들을 제안합니다.
interpolation에 대한 처리 (예:ellipsis)특히 매개변수를 받는 케이스가 어렵습니다. 매개변수가 없다면 스타일 시트에 유틸리티 클래스를 추가하여 대응하는 방법이 있겠지만, 매개변수가 있는 케이스에서는 런타임에 CSS-in-JS처럼 스타일 시트 조작이 어려우므로 기존 방법으로는 처리가 불가능합니다. 기존처럼 interpolation을 기존 스타일에 끼워넣는 방식을 해당 interpolation 스타일의 책임을 가진 컴포넌트를 구현하는 방식으로 변경하여 대응합니다.
styled함수 사용styled함수 자체는 styled-components에서 import 하도록 변경하여 마이그레이션에 문제가 없지만, 함수 내에서 foundation 객체에 접근하는 케이스에 대한 대응이 필요합니다. foundation에 접근하는 케이스를 찾아서 CSS Variable로 마이그레이션하는 코드 작성이 필요합니다.interpolation사용컴포넌트의
interpolation인터페이스를 통해 특정 스타일을 컴포넌트에 주입하고 있는 케이스입니다. 베지어 컴포넌트가 styled-component를 사용하지 않을 것이기 때문에,interpolation인터페이스는 deprecated할 예정입니다. 따라서 className을 통해 동일한 스타일을 구현하든, 컴포넌트를 재작성하든지 처리가 필요합니다. 컴포넌트마다 이 방법은 다를 수 있어, 심도 깊은 고민이 필요합니다.또한 기존
interpolation은 보통 해당 스타일의 가장 '하단'에 주입됩니다. 이는 사용자가 오버라이드하려는 스타일이 가장 높은 우선순위를 가지고 적용되어야 한다는 동작을 내포하고 있습니다. 이는 CSS Cascade Layer로 해결할 수 있습니다. 컴포넌트의 스타일을 특정 레이어(@layer components)에 가둠으로써, 사용자의 스타일이 항상 우선순위를 가지도록 할 수 있습니다.as속성어렵지 않게 구현 가능합니다. 기능 구현보다도, #906 이 잘 적용되도록 만드는 것이 중요합니다만 이또한 어렵지 않을 거로 예상됩니다.
References
The text was updated successfully, but these errors were encountered: