Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify styles globally #1975

Merged
merged 32 commits into from
Feb 7, 2024
Merged

Conversation

sungik-choi
Copy link
Contributor

@sungik-choi sungik-choi commented Feb 4, 2024

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

Summary

Modify styles globally

Details

Bug Fix

  • flex property of LegacyStackItem is invalid #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를 바로 생성해주는 게 가장 좋아보이는데 말이죠. 예를 들어 아래와 같은 부분입니다.
$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});
  }
}
  • Remove the CSS-in-JS library #1106 에서 CSS modules를 선택했던 이유가 외부 의존성을 제거하기 위해서였기에 기대치를 충분히 잘 충족하는 선택이었다고 생각합니다. 하지만 더 나아가서 추후 css 사이즈 등을 더 신경써야한다면 atomic class 기반의 스타일 시스템(e.g. tailwind, panda css, stylex)을 내부적으로만 사용하는 걸 고려해봐야할 수도 있을 거 같아요.
  • 다행히 이번 마이그레이션을 통해 특정 라이브러리의 의존성을 제거했기 때문에, 언제가 될진 모르겠지만 그 때가 온다면, 그 때는 Breaking change 없이 점진적으로 마이그레이션을 잘 진행할 수 있을 거 같아요.

Breaking change? (Yes/No)

No

References

Reset CSS를 포함해야하는지, 포함한다면 어디까지 포함해야할까?

@sungik-choi sungik-choi added enhancement Issues or PR related to making existing features better fix PR related to bug fix labels Feb 4, 2024
@sungik-choi sungik-choi self-assigned this Feb 4, 2024
Copy link

changeset-bot bot commented Feb 4, 2024

⚠️ No Changeset found

Latest commit: 8ad024c

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

Copy link
Contributor

github-actions bot commented Feb 4, 2024

Chromatic Report

🚀 Congratulations! Your build was successful!

Copy link

codecov bot commented Feb 4, 2024

Codecov Report

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

Comparison is base (17eb321) 84.02% compared to head (8ad024c) 84.24%.

Files Patch % Lines
...ezier-react/src/components/TextField/TextField.tsx 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #1975      +/-   ##
==========================================
+ Coverage   84.02%   84.24%   +0.21%     
==========================================
  Files         141      143       +2     
  Lines        2266     2278      +12     
  Branches      610      606       -4     
==========================================
+ Hits         1904     1919      +15     
+ Misses        285      282       -3     
  Partials       77       77              

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

@sungik-choi sungik-choi marked this pull request as ready for review February 5, 2024 13:14
@sungik-choi sungik-choi marked this pull request as draft February 6, 2024 02:50
@sungik-choi sungik-choi marked this pull request as ready for review February 6, 2024 08:04
@@ -9,12 +9,12 @@
}

.FormLabelWrapper {
&:where(.position-top) {
&.position-top {
Copy link
Collaborator

@yangwooseong yangwooseong Feb 7, 2024

Choose a reason for hiding this comment

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

기존에 where 를 사용한 것 때문에 스타일링에 이슈가 있었나요? 스토리북에서 position 을 바꾸면 잘 적용되는 것처럼 보여서 궁금해서 질문드립니다! (#1981 기준 스토리북)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AS-IS TO-BE
image image

labelPosition=left 인 케이스에서 오버라이드가 안되는 문제가 있었어요.

@@ -0,0 +1,12 @@
:where(.UnstyledButton) {
cursor: pointer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

button element 라서 없어도 되지 않나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cursor: pointer는 기본적으로 anchor element에만 적용됩니다!

@sungik-choi sungik-choi merged commit 9eade79 into channel-io:alpha Feb 7, 2024
11 checks passed
@sungik-choi sungik-choi deleted the feat/refine-styles branch February 7, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues or PR related to making existing features better fix PR related to bug fix
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

flex property of LegacyStackItem is invalid
2 participants