-
Notifications
You must be signed in to change notification settings - Fork 11
feat: add data-slots to every components
#703
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
Changes from all commits
0896d48
00de410
d918c3e
8b247e4
5b361ae
f60ff1d
0047235
8e7efeb
a95fb75
7729185
c674ee3
94af759
dc119dd
d5f29ef
c53fe07
ee1b78a
9c85d19
525240f
1258eb9
fec56fc
48d53d5
7a7e618
a716b09
94844fd
a309585
9a8e0e5
f29bcf7
193e20f
8bbc9b1
bc16436
36d8651
22edd97
3c8edf7
83e4a76
91c48c3
8776117
4bef67b
04e9e77
1989a30
af65f87
55fb0a4
a89a31b
e0ad52a
5cf01fc
73079bf
202d52b
2589af6
0d1dbbe
7172c1b
da37aa6
0be04f5
6843654
42b39e1
3d04478
6f0d3f4
0840709
65c2c28
0adef44
23a7dcf
c9b7803
008b144
74a8557
7d12595
0df1a48
09616d7
412943c
e420566
55298bc
01b6fcf
3c52055
2be4c40
36e19cc
c6dcb7b
ed7a6e0
74085a2
789258e
6d5eb6f
e729d00
2f6836a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@vapor-ui/core': minor | ||
| --- | ||
|
|
||
| add data-slots for all components |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -120,7 +120,7 @@ export const PaginationRootPrimitive = forwardRef<HTMLElement, PaginationRootPri | |||||||||||
| return <PaginationProvider value={context}>{element}</PaginationProvider>; | ||||||||||||
| }, | ||||||||||||
| ); | ||||||||||||
| PaginationRootPrimitive.displayName = 'PaginationRootPrimitive.Root'; | ||||||||||||
| PaginationRootPrimitive.displayName = 'Pagination.RootPrimitive'; | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
이렇게 지금 displayName이 없거나 컴포넌트 alias와 다른 케이스가 존재하는 것 같습니다. displayName으로 data-slots이름을 지정하는 방법과 component export index.parts.ts를 통해 alias 이름을 가져와서 data-slots을 넣어주는 방법이 있었을 것 같은데 displayName을 채택한 이유가 궁금합니다.! 저는 export alias를 사용하면 사용단 사용 slot이름과 data-slots의 이름이 동일하게 갈 것 같다고 생각해서 리뷰 남깁니다.!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. export alias 방식은 AST에서 대상을 선택하고 data-slots를 적용하는 부분과, 그것의 값을 꺼내오는 부분이 서로 달라 매핑 과정이 필요하다고 판단했습니다. 그래서 코드가 복잡해지고, 유지보수 비용이 커질 것을 우려해서 선택하지 않았어요! 그리고 지금 컴포넌트의 네이밍이나 alias를 지정하는 규칙 자체는 명확하게 지정되어 있긴 하니, 비용을 줄이되 컨벤션에 따라 값을 뽑아내는 방식은 어떠신가요?? 컴포넌트의 이름에서 파일명 이후에 ex)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 좋은 것 같습니다.! 그럼 파일명 기반으로 서브 컴포넌트가 (.)뒤에 들어가면 명확해 질 것 같네요! 위에 언급한 callout, toolbar, grid, segmentedControl의 displayName은 다른 PR에서 차차 수정하면 될 것 같네요 그럼 |
||||||||||||
|
|
||||||||||||
| /* ------------------------------------------------------------------------------------------------- | ||||||||||||
| * Pagination.ListPrimitive | ||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vapor-ui/core의 컴포넌트 파일을 보는게 아니라 dist를 보게 되면서 스토리북 실행할 때 turbo.json 태스크 의존성을 지정해 주면 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 스토리북 실행 시점의 문제라기보다는, 어차피 core의 dist가 없으면 개발 환경에서 모듈을 찾을 수 없기 때문에 해결될 해결이 충분히 될 문제로 보이긴 합니다..!
그래서 매번 파이프라인을 태우기보다 website의 스크립트들처럼 사용자가 직접 파이프라인을 시작할 수 있도록 구성해두겠습니다!
pnpm turbo dev --filter=storybook