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

Convert enum type of component props to string literal type - Button, Banner, etc #1984

Merged

Conversation

yangwooseong
Copy link
Collaborator

@yangwooseong yangwooseong commented Feb 7, 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

  • 컴포넌트의 enum 타입을 string literal 타입으로 변경하고 codemod 에 변경되는 속성을 추가합니다.
  • 한 pr 에 하기에는 변경사항이 많아서 2~3개로 분리해서 작업하려고 합니다.

Details

  • v2-enum-member-to-string-literal codemod 를 사용해서 하나씩 변환했습니다.
  • 스토리북에서 enum 으로부터 control option 을 얻어오는 부분은 string literal 타입이 되면 스토리북이 알아서 radio 버튼으로 바꿔주기 때문에 제거했습니다. 개발 모드에서는 input type으로 되어 직접 입력해야하지만, 크게 불편하지는 않다고 생각됩니다.
  • StatusType 만 'OnlineCrescent' 처럼 PascalCase 로 되어있고 다른 부분은 kebab-case 라서 변환하면서 모두 kebab-case 로 통일해야 하나 고민입니다. 일관성 측면에서 통일하는게 좋아보입니다.

Breaking change? (Yes/No)

  • Yes

References

  • None

@yangwooseong yangwooseong self-assigned this Feb 7, 2024
Copy link

changeset-bot bot commented Feb 7, 2024

🦋 Changeset detected

Latest commit: 38c5495

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-codemod Minor
@channel.io/bezier-react Major

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

@yangwooseong yangwooseong added enhancement Issues or PR related to making existing features better bezier-codemod Issue or PR related to bezier-codemod labels Feb 7, 2024
Copy link
Contributor

github-actions bot commented Feb 7, 2024

Chromatic Report

🚀 Congratulations! Your build was successful!

@yangwooseong yangwooseong marked this pull request as ready for review February 7, 2024 09:55
Copy link

codecov bot commented Feb 7, 2024

Codecov Report

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

Comparison is base (1e7de8a) 84.23% compared to head (be341d6) 84.04%.

Files Patch % Lines
...ages/bezier-react/src/components/Status/Status.tsx 0.00% 3 Missing ⚠️
.../bezier-react/src/components/ListItem/ListItem.tsx 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #1984      +/-   ##
==========================================
- Coverage   84.23%   84.04%   -0.19%     
==========================================
  Files         142      134       -8     
  Lines        2277     2250      -27     
  Branches      606      606              
==========================================
- Hits         1918     1891      -27     
  Misses        282      282              
  Partials       77       77              

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

@sungik-choi
Copy link
Contributor

스토리북에서 enum 으로부터 control option 을 얻어오는 부분은 string literal 타입이 되면 스토리북이 알아서 radio 버튼으로 바꿔주기 때문에 제거했습니다. 개발 모드에서는 input type으로 되어 직접 입력해야하지만, 크게 불편하지는 않다고 생각됩니다.

👍

StatusType 만 'OnlineCrescent' 처럼 PascalCase 로 되어있고 다른 부분은 kebab-case 라서 변환하면서 모두 kebab-case 로 통일해야 하나 고민입니다. 일관성 측면에서 통일하는게 좋아보입니다.

저도 케밥 케이스로 통일하는 편이 좋을 거 같아요.

"@channel.io/bezier-react": major
---

## "@channel.io/bezier-react": major
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## "@channel.io/bezier-react": major

@@ -75,7 +71,7 @@ export const Avatar = forwardRef<HTMLDivElement, AvatarProps>(function Avatar({
return null
}

const statusSize = size >= AvatarSize.Size90 ? StatusSize.L : StatusSize.M
const statusSize = Number(size) >= 90 ? 'l' : 'm'
Copy link
Contributor

Choose a reason for hiding this comment

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

형변환 하지 않고 명시적으로 맵핑해주는 게 좋을 거 같아요. 이렇게 변경할 경우, 100 as AvatarSize 같은 식으로 사용한 곳에서 제대로 동작하지 않게 될텐데 그건 사용처 잘못이라고 봅니다

@@ -209,7 +212,7 @@ export const AvatarGroup = forwardRef<HTMLDivElement, AvatarGroupProps>(function
)}
style={{
'--b-avatar-group-spacing': px(spacing),
'--b-avatar-group-size': px(size),
'--b-avatar-group-size': cssDimension(size),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'--b-avatar-group-size': cssDimension(size),
'--b-avatar-group-size': `${size}px`

스냅샷 변경사항을 참고해주세요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

size 를 css variable 로 받을 이유는 없어 보여서 classname 으로 바꿨습니다!

@yangwooseong yangwooseong force-pushed the feat/enum-to-string-literal-1 branch from 2b41e5d to 7da63f9 Compare February 8, 2024 06:42
@@ -72,7 +72,7 @@ interface ButtonOwnProps {

/**
* The style variant.
* @default ButtonStyleVariant.Primary
* @default "primary"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아쉽게도 string literal type 은 enum 과 달리 이런 jsdoc을 미리보기로 보여줄 수 없네요

Copy link
Contributor

Choose a reason for hiding this comment

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

네, enum이 jsdoc이나 코드 검색 시에는 확실한 장점이 있어요 (#1568)

@yangwooseong yangwooseong force-pushed the feat/enum-to-string-literal-1 branch from 7da63f9 to 1124241 Compare February 8, 2024 06:44
@yangwooseong
Copy link
Collaborator Author

스토리북에서 enum 으로부터 control option 을 얻어오는 부분은 string literal 타입이 되면 스토리북이 알아서 radio 버튼으로 바꿔주기 때문에 제거했습니다. 개발 모드에서는 input type으로 되어 직접 입력해야하지만, 크게 불편하지는 않다고 생각됩니다.

👍

StatusType 만 'OnlineCrescent' 처럼 PascalCase 로 되어있고 다른 부분은 kebab-case 라서 변환하면서 모두 kebab-case 로 통일해야 하나 고민입니다. 일관성 측면에서 통일하는게 좋아보입니다.

저도 케밥 케이스로 통일하는 편이 좋을 거 같아요.

이번 pr 에 포함했습니다!

@yangwooseong yangwooseong force-pushed the feat/enum-to-string-literal-1 branch from 1124241 to be341d6 Compare February 8, 2024 06:45
Comment on lines 57 to 62
@each $size in $avatar-sizes {
@if $size > 24 {
&:where(.size-#{$size}) {
--b-avatar-group-ellipsis-pr: 6px;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

지금 보니 &:where(.size-30, .size-36...) 한 줄로 할 수 있을 거 같네요

@@ -15,4 +15,4 @@ The properties that change are:
- `ListItemVariant`, `ListItemSize`
- `StatusType`, `StatusSize`

When changed to string literal type, it is changed to the value of enum. e.g. `ButtonStyleVariant.MonoChrome` -> `monochrome`
When changed to string literal type, it is changed to the kebab-cased value of enum. e.g. `ButtonStyleVariant.MonoChromeDark` -> `monochrome-dark`, `StatusType.OnlineCrescent` -> `online-crescent`
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@yangwooseong yangwooseong force-pushed the feat/enum-to-string-literal-1 branch from be341d6 to 38c5495 Compare February 8, 2024 07:02
@yangwooseong yangwooseong merged commit 09e466a into channel-io:alpha Feb 8, 2024
9 checks passed
@yangwooseong yangwooseong deleted the feat/enum-to-string-literal-1 branch February 8, 2024 07:07
yangwooseong added a commit that referenced this pull request Feb 13, 2024
…Tooltip, etc (#1990)

<!--
  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 #0000 -->

- Fixes #1815 
- Fixes #1568 

## Summary
<!-- Please brief explanation of the changes made -->

- #1984 의 후속 작업입니다. 
- 컴포넌트의 enum 타입을 string literal 타입으로 변경하고, codemod 에 변경사항을 추가하여 마이그레이션을
지원합니다.

## Details
<!-- Please elaborate description of the changes -->

- 대부분의 경우는 이전 pr과 동일하게 PascalCase 의 enum 을 kebab-case 의 string literal
타입으로 변환하는 작업입니다. 여기서 2가지 경우만 약간의 예외적인 상황이 있었습니다.
- `IconSize`의 경우 `IconSize.Normal = 24`와 같이 size value 를 그대로 사용하던 것을
`'normal'` 을 받고 컴포넌트 내부에서 size 로 맵핑하는 형식으로 변경했습니다. number literal 을 그대로
받는 것보다 이렇게 하는 게 다른 컴포넌트와의 일관성이 유지된다고 생각하였습니다.
- 한편 `TextAreaHeight` enum 은 `TextAreaHeight.Row16` 처럼 쓰이고 있어서 이를
`'row16'` 으로 바꾸는 것보다는 `16`으로 받는게 깔끔한 것 같아서 예외적으로 number literal 을 받도록
했습니다.

### Breaking change? (Yes/No)
<!-- If Yes, please describe the impact and migration path for users -->
- Yes (changeset 참고) 

## References
<!-- Please list any other resources or points the reviewer should be
aware of -->

- None
sungik-choi added a commit that referenced this pull request Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bezier-codemod Issue or PR related to bezier-codemod enhancement Issues or PR related to making existing features better
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants