feat: add data-slots to every components - #703
Conversation
🦋 Changeset detectedLatest commit: 2f6836a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (10)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughRolldown 플러그인이 컴포넌트에 Changesdata-slots 빌드 통합
컴포넌트 구현 조정
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change injects data-slots during the Core build, updates Storybook to consume distribution CSS, and standardizes relevant component wrappers. No current merge-blocking risk is identified. Sequence Diagram(s)sequenceDiagram
participant Rolldown
participant dataSlots
participant ComponentTSX
participant CoreBundle
participant Storybook
Rolldown->>dataSlots: TSX 변환 요청
dataSlots->>ComponentTSX: 컴포넌트와 ref carrier 분석
dataSlots-->>Rolldown: data-slots가 주입된 코드 반환
Rolldown->>CoreBundle: ESM 또는 CJS 번들 생성
Storybook->>CoreBundle: Core 배포 CSS 요청
CoreBundle-->>Storybook: .vanilla.css 파일 반환
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚫 0 tests failed!
Click here if you need to update snapshots. |
| { find: '@vapor-ui/core', replacement: CORE_SRC }, | ||
| { find: '@vapor-ui/composites', replacement: COMPOSITES_SRC }, | ||
| ], | ||
| alias: [tildeAlias], |
There was a problem hiding this comment.
vapor-ui/core의 컴포넌트 파일을 보는게 아니라 dist를 보게 되면서 스토리북 실행할 때 turbo.json 태스크 의존성을 지정해 주면 좋을 것 같습니다.
There was a problem hiding this comment.
이 부분은 스토리북 실행 시점의 문제라기보다는, 어차피 core의 dist가 없으면 개발 환경에서 모듈을 찾을 수 없기 때문에 해결될 해결이 충분히 될 문제로 보이긴 합니다..!
그래서 매번 파이프라인을 태우기보다 website의 스크립트들처럼 사용자가 직접 파이프라인을 시작할 수 있도록 구성해두겠습니다!
- 파이프라인 실행 커맨드:
pnpm turbo dev --filter=storybook
| }, | ||
| ); | ||
| PaginationRootPrimitive.displayName = 'PaginationRootPrimitive.Root'; | ||
| PaginationRootPrimitive.displayName = 'Pagination.RootPrimitive'; |
There was a problem hiding this comment.
| 사용자가 쓰는 것 | DOM에 찍히는 이름표 |
|---|---|
| <Callout.Root> / <Callout.Icon> | CalloutRoot / CalloutIcon |
| <Toolbar.Root> .Group .Button .Input .Separator | ToolbarRoot ToolbarGroup … |
| <Grid.Root> | Grid ← 같은 파일의 Grid.Item은 정상 |
| <SegmentedControl.IconItem> | SegmentedControl.IconOnlyItem |
이렇게 지금 displayName이 없거나 컴포넌트 alias와 다른 케이스가 존재하는 것 같습니다.
displayName으로 data-slots이름을 지정하는 방법과 component export index.parts.ts를 통해 alias 이름을 가져와서 data-slots을 넣어주는 방법이 있었을 것 같은데 displayName을 채택한 이유가 궁금합니다.!
저는 export alias를 사용하면 사용단 사용 slot이름과 data-slots의 이름이 동일하게 갈 것 같다고 생각해서 리뷰 남깁니다.!
There was a problem hiding this comment.
export alias 방식은 AST에서 대상을 선택하고 data-slots를 적용하는 부분과, 그것의 값을 꺼내오는 부분이 서로 달라 매핑 과정이 필요하다고 판단했습니다. 그래서 코드가 복잡해지고, 유지보수 비용이 커질 것을 우려해서 선택하지 않았어요!
그리고 지금 컴포넌트의 네이밍이나 alias를 지정하는 규칙 자체는 명확하게 지정되어 있긴 하니, 비용을 줄이되 컨벤션에 따라 값을 뽑아내는 방식은 어떠신가요??
컴포넌트의 이름에서 파일명 이후에 .을 찍는 방식처럼요!
ex) dialog.tsx에서 DialogPortalPrimitive의 data-slots 값을 계산하는 경우, 파일명(dialog) 다음 부분에 .찍고 Primitive 제거 -> Dialog.Portal
There was a problem hiding this comment.
좋은 것 같습니다.! 그럼 파일명 기반으로 서브 컴포넌트가 (.)뒤에 들어가면 명확해 질 것 같네요!
위에 언급한 callout, toolbar, grid, segmentedControl의 displayName은 다른 PR에서 차차 수정하면 될 것 같네요 그럼
| "@repo/eslint-config": "workspace:*", | ||
| "@repo/typescript-config": "workspace:*", | ||
| "eslint": "catalog:", | ||
| "magic-string": "^0.30.21", |
There was a problem hiding this comment.
magic-string은 번들러 런타임에 남아있는 의존성인데 여기에 들어온 이유가 있을까요?? 추가로 최신 버전은 major 버전인데 해당 버전을 추가하신 이유가 있을까요??
There was a problem hiding this comment.
번들러의 코드 파싱 과정에 개입하여 특정 코드를 추가하는 등의 작업을 해야 하는데, 원본 소스나 매핑 위치 등을 해치지 않고 안정적으로 추가하기 위해서 패키지 자체가 필요하긴 합니다..! https://rolldown.rs/apis/plugin-api/transformations
다만 버전은 업그레이드 해둘게요..! 제가 실수했네요ㅠㅠ
There was a problem hiding this comment.
엇 그래서 번들러 코드 파싱 과정에 필요해서 devDependencies가 아닌 dependencies에 있어야 하는 거 아닌가 해서 코멘트 남긴거긴 했었어요!
There was a problem hiding this comment.
아하 그러네요..! 확인 감사합니다, 수정해두었어요~!
| "rollup-plugin-node-externals": "^8.1.2" | ||
| }, | ||
| "devDependencies": { | ||
| "@oxc-project/types": "^0.147.0", |
There was a problem hiding this comment.
rolldown에서 하위 의존성으로 @oxc-project/types =0.138.0을 지정해서 동일하게 버전을 지정하면 어떨까 합니다. 사용하는 타입에서는 버전 차이에 따른 타입 차이가 없긴 하지만 일부 변경된 코드를 추후에 사용할 가능성을 고려하면 수정하는 것이 어떨까 합니다.!
There was a problem hiding this comment.
그럼 이건 catalog로 등록해두겠습니다!
버전을 고정해버리면 rolldown 버전이 올라갈 때 놓치고 넘어갈 수도 있으니, 아예 묶어서 같이 관리해버리면 그런 문제를 어느정도 해결할 수 있을 것 같아서요!
There was a problem hiding this comment.
그리고 확인한 김에 rolldown과 @oxc-project/types 모두 버전 최신화 해서 카탈로그에 등록해둘게요!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@MaxLee-dev 이거 오전에 이야기 했던 http-server 문제로 회귀 테스트만 실패하고 있는데, 문제 해결해둔 PR이 여기에 의존성 걸려 있어서 우선 머지하도록 할게요! |
3 participants
Description of Changes
Summary by CodeRabbit
data-slots속성이 추가되어 슬롯 기반 스타일링과 요소 식별이 쉬워졌습니다.@vapor-ui/core의 마이너 버전 업데이트로 제공됩니다.data-slots속성을 추가하도록 플러그인 추가해뒀습니다.data-slots라는 네이밍을 사용한 이유는 프로젝트 내부에서slots이라는 명칭을 비슷한 용도로 사용하고 있어서 통일하면 좋겠다고 생각했어요.--
참고사항
번들링 단계에서 data-slots 속성을 추가하는 것이다 보니 스토리북에서는 이 속성들을 확인할 수가 없습니다..!
개발 편의를 위해 스토리북에서는 core와 composites을 alias 경로로 해석하여 사용하고 있습니다.
@vapor-ui/core패키지의*.vanilla.css파일을 만나면 Vanilla Extract 플러그인이 이 CSS 파일을 다시 한 번 컴파일 하려고 시도하기 때문에 에러가 발생했습니다.*.vanilla.css파일을 만나면 컴파일하지 않고 넘어가도록coreDistCssPassthrough플러그인을 따로 추가해뒀습니다.Sheet에서 Dialog를 그대로 재할당하는 부분이 있는데, 이는 AST 분석 시 다른 컴포넌트들과 다르게 분석되고 있었습니다.
Checklist
Before submitting the PR, please make sure you have checked all of the following items.