Migrate Tooltip, Help component with scss
#1862
Conversation
|
Chromatic Report🚀 Congratulations! Your build was successful! |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## alpha #1862 +/- ##
==========================================
+ Coverage 86.24% 87.07% +0.82%
==========================================
Files 272 270 -2
Lines 3745 3745
Branches 784 787 +3
==========================================
+ Hits 3230 3261 +31
+ Misses 434 402 -32
- Partials 81 82 +1 ☔ View full report in Codecov by Sentry. |
yangwooseong
added
the
documentation
| collisionPadding={8} | ||
| hideWhenDetached | ||
| > | ||
| <Styled.TooltipContent forwardedAs={as}> |
yangwooseong
force-pushed
the
feat/sass/tooltip
branch
from
70b11f9
to
d67a66c
Compare
| hideWhenDetached | ||
| > | ||
| <HStack spacing={4} className={styles['tooltip-content']}> | ||
| <div className={styles['text-container']}> |
There was a problem hiding this comment.
베지어 내에서도 이런 것들을 Box 를 사용해서 만드는 게 좋을까요? @sungik-choi
There was a problem hiding this comment.
저도 고민입니다. 성능상 이점은 Box를 쓰지 않는 쪽이 런타임 오버헤드가 없어 좋은데, Box에 구현되어있는 스타일(class)를 중복해서 구현하는 거 같기도 하구요. 어떻게 생각하시나요?
There was a problem hiding this comment.
개인적으로는 Box 를 외부로 노출시키기 전에 내부에서 의존성을 만들어도 될지 의문이 들기는 합니다.
There was a problem hiding this comment.
일단은 Box 없이 구현하는 거로 하시죠. 나중에 필요하다면 리팩토링하면 되니까요
yangwooseong
added
enhancement
| @@ -6,6 +6,7 @@ import { | |||
| } from '@testing-library/react' | |||
| import userEvent from '@testing-library/user-event' | |||
|
|
|||
| import { LightThemeProvider } from '~/src/providers/ThemeProvider' | |||
| import { render } from '~/src/utils/test' | |||
There was a problem hiding this comment.
개별 테스트에서 Provider를 추가하기보다 render 의 TestProvider 에서 AppProvider를 추가하는 게 좋을 거 같아요
| line-height: 0; | ||
|
|
||
| &:not([data-state="closed"]) { | ||
| > .Icon { |
There was a problem hiding this comment.
호버 스타일이 제대로 적용되지 않고 있어요
| > .Icon { | |
| & > .icon { |
There was a problem hiding this comment.
Icon.styled.ts 에서 만들어진 color 스타일이 unlayered 라서, components 레이어 밑에 있는 tooltip 스타일이 오버라이딩 되고 있던 것이 원인인듯합니다. Icon 컴포넌트 적용사항 반영하니 잘 되네요!
(suggestion 주신 코드에서 & 쓰는 거랑 안쓰는 거랑 차이 없는 걸로 이해했습니다)
There was a problem hiding this comment.
👍
- 제안은
.icon으로 드렸는데, 작성해주신대로.Icon이 좋을듯..! - 약간 컨텍스트가 다를 수도 있는데, 저도 SegmentedControl 작업하다보니 components layer 내부에 포함돼있다라도 스타일 우선순위가 일부 있네요. radix theme처럼 :where 수도 클래스 적극적으로 사용해서 우선순위를 최대한 낮추는게 중요할 거 같아요
| */ | ||
| margin: 1px; | ||
| } | ||
| } |
| hideWhenDetached | ||
| > | ||
| <HStack spacing={4} className={styles['tooltip-content']}> | ||
| <div className={styles['text-container']}> |
There was a problem hiding this comment.
저도 고민입니다. 성능상 이점은 Box를 쓰지 않는 쪽이 런타임 오버헤드가 없어 좋은데, Box에 구현되어있는 스타일(class)를 중복해서 구현하는 거 같기도 하구요. 어떻게 생각하시나요?
| @use "../../styles/mixins/typography"; | ||
|
|
||
| @layer components { | ||
| .tooltip-content { |
There was a problem hiding this comment.
| .tooltip-content { | |
| .Tooltip { |
아직 명확한 컨벤션은 없으나, module의 root class는 컴포넌트명이랑 통일하면 좋을 거 같아요!
There was a problem hiding this comment.
SegmentedControl 마이그레이션해보니까.. 아래와 같이 네이밍 규칙을 정하면 어떨까 싶네요
- 컴포넌트에 적용되는 root class는 파스칼 케이스로 한다
- 상태에 따른 nested class는 케밥 케이스로 한다
- 처음에
styles.fooStyle같은 식으로 사용하기 편해서 카멜 케이스를 고려해봤는데,styles[display-${display}]같은 식으로 케밥 케이스가 케이스 변환없이 사용하기 편한 장점이 있네요. Laying the groundwork for good handling of unused CSS #1850 이슈를 고려해보면 카멜 케이스가 좋을 거 같은데(computed key를 사용하지 않는 방향), 나중에 적용해도 괜찮지 않을까.. 싶었습니다
- 처음에
| } | ||
|
|
||
| .content { | ||
| @include typography.ellipsis(20, var(--font-line-height-18)); |
There was a problem hiding this comment.
mixin대신, Text 의 truncated prop을 확장하는 건 어떨까요? 이 PR의 컨텍스트와 약간 벗어나는 부분이긴 한데요.
기존 구현에서 인자로 전달하는 line-height가 typography에 의존적인데, 이 문제도 해결할 수 있을 거 같아요.
(+ 이 변경사항이 적용되지 않더라도, var(--typography-size-13-line-height) 가 더 적합한 토큰일 거 같음)
truncated: boolean | number (line-clamp value).Text {
--b-text-line-clamp: 1; /* inline style로 주입됨 */
--b-text-line-height: inherit; /* initial도 가능. typo가 required prop이므로 큰 의미는 없는 기본값 */
line-height: var(--b-text-line-height);
&.typo-11 {
font-size: var(--typography-size-11-font-size);
--b-text-line-height: var(--typography-size-11-line-height);
}
/* ...typo styles... */
&.multi-line-truncated { /* truncated가 1 이상이면 조건부 활성화..? */
display: -webkit-box;
max-height: calc(var(--b-text-line-clamp) * var(--b-text-line-height));
overflow: hidden;
line-height: var(--b-text-line-height);
text-overflow: ellipsis;
-webkit-box-orient: vertical;
-webkit-line-clamp: var(--b-text-line-clamp);
}
}There was a problem hiding this comment.
좋은 것 같습니다! 앞으로는 ellipsis mixin을 제거하고 Text 컴포넌트에 모두 위임하는 게 의도가 맞을까요?
There was a problem hiding this comment.
SegmentedControl 작업하다보니 모든 케이스를 Text로 대체할 수는 없네요...
일단은 최대한 위임하는 쪽으로 생각중입니다..!
yangwooseong
force-pushed
the
feat/sass/tooltip
branch
from
d67a66c
to
3964ea3
Compare
| @@ -0,0 +1,11 @@ | |||
| @layer components { | |||
| .Trigger { | |||
| line-height: 0; | ||
|
|
||
| &:not([data-state="closed"]) { | ||
| > .Icon { |
There was a problem hiding this comment.
👍
- 제안은
.icon으로 드렸는데, 작성해주신대로.Icon이 좋을듯..! - 약간 컨텍스트가 다를 수도 있는데, 저도 SegmentedControl 작업하다보니 components layer 내부에 포함돼있다라도 스타일 우선순위가 일부 있네요. radix theme처럼 :where 수도 클래스 적극적으로 사용해서 우선순위를 최대한 낮추는게 중요할 거 같아요
| @use "../../styles/mixins/typography"; | ||
|
|
||
| @layer components { | ||
| .tooltip-content { |
There was a problem hiding this comment.
SegmentedControl 마이그레이션해보니까.. 아래와 같이 네이밍 규칙을 정하면 어떨까 싶네요
- 컴포넌트에 적용되는 root class는 파스칼 케이스로 한다
- 상태에 따른 nested class는 케밥 케이스로 한다
- 처음에
styles.fooStyle같은 식으로 사용하기 편해서 카멜 케이스를 고려해봤는데,styles[display-${display}]같은 식으로 케밥 케이스가 케이스 변환없이 사용하기 편한 장점이 있네요. Laying the groundwork for good handling of unused CSS #1850 이슈를 고려해보면 카멜 케이스가 좋을 거 같은데(computed key를 사용하지 않는 방향), 나중에 적용해도 괜찮지 않을까.. 싶었습니다
- 처음에
| } | ||
|
|
||
| .content { | ||
| @include typography.ellipsis(20, var(--font-line-height-18)); |
There was a problem hiding this comment.
SegmentedControl 작업하다보니 모든 케이스를 Text로 대체할 수는 없네요...
일단은 최대한 위임하는 쪽으로 생각중입니다..!
yangwooseong
force-pushed
the
feat/sass/tooltip
branch
2 times, most recently
from
dbf8b4b
to
693b702
Compare
| @@ -47,11 +47,13 @@ export const Text = forwardRef<HTMLElement, TextProps>(function Text(props, forw | |||
| align, | |||
| ...rest | |||
| } = marginRest | |||
| const isMultiLineTruncated = typeof truncated === 'number' && truncated >= 1 | |||
There was a problem hiding this comment.
| const isMultiLineTruncated = typeof truncated === 'number' && truncated >= 1 | |
| const isMultiLineTruncated = isNumber(truncated) && truncated >= 1 |
| font-size: var(--typography-size-11-font-size); | ||
| line-height: var(--typography-size-11-line-height); | ||
| --b-text-line-height: var(--typography-size-11-line-height); |
There was a problem hiding this comment.
| font-size: var(--typography-size-11-font-size); | |
| line-height: var(--typography-size-11-line-height); | |
| --b-text-line-height: var(--typography-size-11-line-height); | |
| --b-text-font-size: var(--typography-size-11-font-size); | |
| --b-text-line-height: var(--typography-size-11-line-height); |
--b-text-color: inherit;
--b-text-line-clamp: 1;
--b-text-font-size: initial;
--b-text-line-height: initial;
--b-text-letter-spacing: initial;
.Text {
font-size: var(--b-text-font-size);
line-height: var(--b-text-line-height);
letter-spacing: var(--b-text-letter-spacing);
/* ... */
}이렇게 하면 line-height의 중복을 줄일 수 있을 거 같아요.
| truncated === true && styles.truncated, | ||
| isMultiLineTruncated && styles['multi-line-truncated'], |
There was a problem hiding this comment.
| truncated === true && styles.truncated, | |
| isMultiLineTruncated && styles['multi-line-truncated'], | |
| truncated === true | |
| ? styles.truncated | |
| : isMultiLineTruncated && styles['multi-line-truncated'], |
styles.truncated, styles['multi-line-truncated'] 두 클래스의 배타성이 잘 드러나면 더 좋을 거 같아요.
| @@ -62,9 +62,10 @@ interface TextOwnProps { | |||
| italic?: boolean | |||
| /** | |||
| * Whether the text is truncated. | |||
| * If it is a number type, it means the maximum number of lines. | |||
There was a problem hiding this comment.
'positive' number(integer)로 범위를 더 좁혀서 적어주면 좋을 거 같아요!
| <Icon | ||
| size={IconSize.XS} | ||
| color="txt-black-darkest" | ||
| m={1} |
There was a problem hiding this comment.
margin이 아니라 padding 1px이 되어야할 거 같습니다. 외적으로는 같은데, 디자인이 마진이 아니라 패딩으로 구현되어있었어서 그걸 따르는게 좋을 거 같아요.
| .TooltipContainer { | ||
| overflow: hidden; | ||
| } | ||
|
|
||
| .TooltipContent { | ||
| white-space: pre-wrap; | ||
| } |
There was a problem hiding this comment.
.Tooltip {}
.TooltipContainer {}
.TooltipContent {}가능하면 플랫하게 펼쳐서 선택자 우선순위를 낮추는게 좋을 거 같아요
| @@ -0,0 +1,11 @@ | |||
| @layer components { | |||
| .Trigger { | |||
…vider throws error otherwise
…uncated classname exclusively
yangwooseong
force-pushed
the
feat/sass/tooltip
branch
from
789fb1f
to
80da6b1
Compare
Self Checklist
Related Issue
Summary
Tooltip,Help컴포넌트를 scss 로 마이그레이션합니다.Details
Tooltip은 이미 offset 속성으로 트리거 대상과 얼마나 떨어질지를 지정할 수 있어서MarginProps를 지원하지는 않았습니다.Icon컴포넌트가 scss 로 변환되기 전이라 관련 스타일링이 제대로 적용되지 않았습니다. color 가 inverted color 로 안되고 margin 이 제대로 적용되기 전입니다.Iconcomponent with scss #1863 rebase 후 스타일링 확인했습니다Breaking change? (Yes/No)
References