refactor: JDS Hero/Title/Label 컴포넌트 제거#441
Open
ccconac wants to merge 17 commits into
Open
Conversation
@jects/jds의 Hero, Title, Label을 로컬 typography 컴포넌트로 대체하고, color prop을 className 기반 Tailwind 클래스로 변경
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ccconac
changed the title
ccconac
changed the title
9 tasks
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughJDS의 Label/Hero/Title 컴포넌트를 제거하고 새로운 typography 유틸 시스템을 구축한 후, apps/web에 로컬 대체 컴포넌트를 추가하여 모든 JDS typography 의존성을 제거하는 리팩토링입니다. ChangesTypography 시스템 리팩토링 및 마이그레이션
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/jds/src/components/Menu/Menu/Menu.tsx (1)
42-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
MenuCategory의 ref 타입을 실제 요소와 일치시켜주세요.Line 42에서
forwardRef<HTMLDivElement, MenuCategoryProps>로 선언되어 있지만,StyledMenuCategory는styled("span")로 구현되어 있습니다. ref 타입 제네릭이 렌더되는 실제 DOM 요소와 맞지 않습니다.제안 수정
-const MenuCategory = forwardRef<HTMLDivElement, MenuCategoryProps>(({ children, ...rest }, ref) => { +const MenuCategory = forwardRef<HTMLSpanElement, MenuCategoryProps>(({ children, ...rest }, ref) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jds/src/components/Menu/Menu/Menu.tsx` around lines 42 - 47, The forwardRef generic for MenuCategory doesn't match the rendered element: MenuCategory uses forwardRef<HTMLDivElement, MenuCategoryProps> but renders StyledMenuCategory which is a styled("span"). Update the ref generic to the actual DOM element type (use HTMLSpanElement) by changing the forwardRef declaration for MenuCategory to forwardRef<HTMLSpanElement, MenuCategoryProps>, ensuring ref typing aligns with StyledMenuCategory; alternatively, if you intended a div, change StyledMenuCategory to render a div instead—pick the approach that preserves the intended DOM element.apps/web/src/pages/ApplyGuidePage.tsx (1)
3-18:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
Label을 로컬 typography에서 가져오도록 수정 필요합니다.ApplyGuidePage.tsx의 line 8에서
Label을@jects/jds에서 가져오고 있으나, JDS는Label을 내보내지 않습니다.Label은@/components/common/typography에서 이미 구현되어 있으며,Hero와Title도 동일한 위치에서 가져오고 있습니다. 현재 상태는 import 오류를 야기합니다.수정 제안
import { Accordion, BlockButton, Divider, IconButton, - Label, LocalNavigation, Tab, toastController, } from "`@jects/jds`"; import { theme } from "`@jects/jds/tokens`"; import { useNavigate, useParams, useSearchParams, Navigate } from "react-router-dom"; import type { JobFamily } from "`@/apis/apply`"; -import { Hero, Title } from "`@/components/common/typography`"; +import { Hero, Label, Title } from "`@/components/common/typography`";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/pages/ApplyGuidePage.tsx` around lines 3 - 18, The import list in ApplyGuidePage.tsx incorrectly imports Label from "`@jects/jds`" causing an import error; update the imports so that Label is imported from the local typography module alongside Hero and Title (i.e., move Label from the "`@jects/jds`" import to the "`@/components/common/typography`" import) and remove Label from the destructured imports from "`@jects/jds`" to match available exports.
🧹 Nitpick comments (3)
packages/jds/src/components/Toast/toast.styles.ts (1)
79-88: 💤 Low value
&&선택자를 사용한 specificity 증가를 고려하세요.
&&선택자로 색상 적용 우선순위를 높이고 있습니다. 이 패턴은getLabelClassName으로 생성된 className 스타일을 override하기 위한 의도로 보이지만, CSS specificity에 의존하는 방식은 향후 유지보수 시 스타일 충돌 가능성을 높일 수 있습니다.다만 코드베이스 전반에 걸쳐 일관되게 사용되는 패턴이므로, 현재로서는 문제없습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jds/src/components/Toast/toast.styles.ts` around lines 79 - 88, The ToastLabel styled component currently uses the "&&" selector to force higher CSS specificity and override styles produced by getLabelClassName; instead remove the "&&" hack and resolve precedence by composing/merging the generated class or by applying the toast style color via the same class generation path (e.g., update getLabelClassName to include color from toastStylesMap or pass a className prop to ToastLabel), ensuring ToastLabel uses toastStylesMap(theme)[toastStyle].color without relying on "&&" so styles are deterministic and maintainable.apps/web/src/components/common/typography/Label.tsx (1)
10-18: ⚖️ Poor tradeoff
htmlForprop의 타입 안전성 문제
htmlFor가 props에 항상 포함되지만,asprop으로label이 아닌 다른 요소를 렌더링할 경우에도 해당 prop이 전달되어 유효하지 않은 HTML attribute가 생성될 수 있습니다.🔧 조건부 타입으로 개선
고급 타입 기법을 사용하여
as="label"일 때만htmlFor를 허용하도록 개선할 수 있습니다:+type LabelPropsBase = { + as?: ElementType; + size?: LabelSize; + textAlign?: LabelAlign; + weight?: LabelWeight; + cursor?: LabelCursor; + children: ReactNode; +}; + +type LabelPropsWithHtmlFor = LabelPropsBase & { + as?: "label"; + htmlFor?: string; +}; + +type LabelPropsWithoutHtmlFor = LabelPropsBase & { + as: Exclude<ElementType, "label">; + htmlFor?: never; +}; + +type LabelProps = (LabelPropsWithHtmlFor | LabelPropsWithoutHtmlFor) & + Omit<HTMLAttributes<HTMLElement>, "style">;또는 간단하게
htmlFor를 제거하고 사용처에서 직접 spread할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/common/typography/Label.tsx` around lines 10 - 18, LabelProps currently always includes htmlFor which can produce invalid attributes when as is not "label"; change the prop typing so htmlFor is allowed only when as extends "label" (e.g., convert LabelProps into a discriminated union with a variant for { as?: "label"; htmlFor?: string } and a variant for other ElementType values that omits htmlFor), or alternatively remove htmlFor from LabelProps and require callers to spread htmlFor themselves when rendering a label; update the Label component signature and any usages to match the new LabelProps shape (refer to LabelProps, the as prop and htmlFor) so non-label elements no longer receive htmlFor.apps/web/src/components/common/typography/Hero.tsx (1)
14-19: ⚖️ Poor tradeoffHero와 Title의 sizeClassName 매핑이 동일한 것은 의도된 설계입니다.
Hero.tsx와 Title.tsx가 동일한 sizeClassName 매핑(lg→title-4, md→title-3 등)을 사용하는 것은 맞습니다. 다만 이 두 컴포넌트의 의미적 차이는 매핑을 통해서가 아니라 기본값과 폰트 굵기를 통해 표현됩니다:
- Hero: 기본 크기="lg"(title-4) + 굵기="boldest"
- Title: 기본 크기="md"(title-3) + 굵기="bolder"
따라서 토큰 매핑을 별도로 분리할 필요는 없으며, 현재 구조는 일관되고 합리적인 설계 패턴입니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/common/typography/Hero.tsx` around lines 14 - 19, The size-to-token mapping in Hero.tsx (sizeClassName) intentionally mirrors Title.tsx; to avoid reviewer confusion, add a concise inline comment near the sizeClassName declaration in Hero.tsx explaining that the identical mapping is deliberate and that semantic differences are expressed via default props and font weight (Hero: default size "lg" and weight "boldest"; Title: default "md" and weight "bolder"), and reference the related Title component so future readers understand the design choice.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/jds/src/components/Input/TextField/TextFieldButton.tsx`:
- Line 43: The label in TextFieldButton uses getLabelClassName({ weight: "bold"
}) without specifying size, causing inconsistent label sizing across
InputArea/SelectField/TagField/FormField which use size: "sm"; update the call
in TextFieldButton (where getLabelClassName is invoked) to include size: "sm"
alongside weight to match the other input components and ensure consistent label
typography.
In `@packages/jds/src/components/Select/SelectCheckbox.tsx`:
- Around line 60-62: The caption typography in SelectCheckbox is missing the
subtle weight, so update the getLabelClassName call in SelectCheckbox (where
size is passed) to also include weight: "subtle" so the caption uses the same
subtle weight as SelectList; modify the props passed to getLabelClassName
accordingly to include weight: "subtle" alongside the existing size logic.
---
Outside diff comments:
In `@apps/web/src/pages/ApplyGuidePage.tsx`:
- Around line 3-18: The import list in ApplyGuidePage.tsx incorrectly imports
Label from "`@jects/jds`" causing an import error; update the imports so that
Label is imported from the local typography module alongside Hero and Title
(i.e., move Label from the "`@jects/jds`" import to the
"`@/components/common/typography`" import) and remove Label from the destructured
imports from "`@jects/jds`" to match available exports.
In `@packages/jds/src/components/Menu/Menu/Menu.tsx`:
- Around line 42-47: The forwardRef generic for MenuCategory doesn't match the
rendered element: MenuCategory uses forwardRef<HTMLDivElement,
MenuCategoryProps> but renders StyledMenuCategory which is a styled("span").
Update the ref generic to the actual DOM element type (use HTMLSpanElement) by
changing the forwardRef declaration for MenuCategory to
forwardRef<HTMLSpanElement, MenuCategoryProps>, ensuring ref typing aligns with
StyledMenuCategory; alternatively, if you intended a div, change
StyledMenuCategory to render a div instead—pick the approach that preserves the
intended DOM element.
---
Nitpick comments:
In `@apps/web/src/components/common/typography/Hero.tsx`:
- Around line 14-19: The size-to-token mapping in Hero.tsx (sizeClassName)
intentionally mirrors Title.tsx; to avoid reviewer confusion, add a concise
inline comment near the sizeClassName declaration in Hero.tsx explaining that
the identical mapping is deliberate and that semantic differences are expressed
via default props and font weight (Hero: default size "lg" and weight "boldest";
Title: default "md" and weight "bolder"), and reference the related Title
component so future readers understand the design choice.
In `@apps/web/src/components/common/typography/Label.tsx`:
- Around line 10-18: LabelProps currently always includes htmlFor which can
produce invalid attributes when as is not "label"; change the prop typing so
htmlFor is allowed only when as extends "label" (e.g., convert LabelProps into a
discriminated union with a variant for { as?: "label"; htmlFor?: string } and a
variant for other ElementType values that omits htmlFor), or alternatively
remove htmlFor from LabelProps and require callers to spread htmlFor themselves
when rendering a label; update the Label component signature and any usages to
match the new LabelProps shape (refer to LabelProps, the as prop and htmlFor) so
non-label elements no longer receive htmlFor.
In `@packages/jds/src/components/Toast/toast.styles.ts`:
- Around line 79-88: The ToastLabel styled component currently uses the "&&"
selector to force higher CSS specificity and override styles produced by
getLabelClassName; instead remove the "&&" hack and resolve precedence by
composing/merging the generated class or by applying the toast style color via
the same class generation path (e.g., update getLabelClassName to include color
from toastStylesMap or pass a className prop to ToastLabel), ensuring ToastLabel
uses toastStylesMap(theme)[toastStyle].color without relying on "&&" so styles
are deterministic and maintainable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4dd2b4fa-0697-4165-b8f1-67b8349aac17
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (117)
apps/web/package.jsonapps/web/src/components/common/typography/Hero.tsxapps/web/src/components/common/typography/Label.tsxapps/web/src/components/common/typography/Title.tsxapps/web/src/components/common/typography/index.tsapps/web/src/components/curriculum/sections/CurriculumTabSection.tsxapps/web/src/components/gnb/Sidebar.tsxapps/web/src/components/main/sections/HeroSection.tsxapps/web/src/components/main/sections/IntroSection.tsxapps/web/src/components/main/sections/JoinSection.tsxapps/web/src/components/vision/sections/GoalSection.tsxapps/web/src/components/vision/sections/GrowthStorySection.tsxapps/web/src/components/vision/sections/MemberSection.tsxapps/web/src/components/vision/sections/ProjectStartSection.tsxapps/web/src/components/vision/sections/VisionHeroSection.tsxapps/web/src/features/apply/steps/CompleteStep.tsxapps/web/src/features/apply/steps/applicationInfo/ApplicantInfoStep.tsxapps/web/src/features/apply/steps/registration/components/QuestionFieldWrapper.tsxapps/web/src/features/shared/components/ApplyStepLayout.tsxapps/web/src/pages/ApplyGuidePage.tsxapps/web/src/pages/ApplyListPage.tsxapps/web/src/pages/Curriculum.tsxapps/web/src/pages/Faq.tsxapps/web/src/pages/LiveSession.tsxapps/web/src/pages/Maintenance.tsxapps/web/src/pages/MiniStudy.tsxapps/web/src/pages/NonSpecificError.tsxapps/web/src/pages/NotFoundError.tsxapps/web/src/pages/TeamProject.tsxapps/web/src/pages/TeamProjectDetail.tsxapps/web/src/utils/cn.tspackages/jds/package.jsonpackages/jds/src/components/Accordion/Accordion.tsxpackages/jds/src/components/Accordion/accordion.styles.tspackages/jds/src/components/Badge/contentBadge/ContentBadge.style.tspackages/jds/src/components/Badge/contentBadge/ContentBadge.tsxpackages/jds/src/components/Badge/numericBadge/NumericBadge.style.tspackages/jds/src/components/Badge/numericBadge/NumericBadge.tsxpackages/jds/src/components/Checkbox/Checkbox.tsxpackages/jds/src/components/Checkbox/checkbox.styles.tspackages/jds/src/components/Dialog/Dialog.styles.tspackages/jds/src/components/Dialog/Dialog.tsxpackages/jds/src/components/EmptyState/EmptyState.tsxpackages/jds/src/components/EmptyState/emptyState.styles.tspackages/jds/src/components/FileItem/FileItem.tsxpackages/jds/src/components/FileItem/fileItem.styles.tspackages/jds/src/components/FileItem/fileItem.types.tspackages/jds/src/components/Hero/Hero.stories.tsxpackages/jds/src/components/Hero/Hero.style.tspackages/jds/src/components/Hero/Hero.tsxpackages/jds/src/components/Hero/index.tspackages/jds/src/components/Image/Image.style.tspackages/jds/src/components/Image/Image.tsxpackages/jds/src/components/Input/InputArea/InputArea.tsxpackages/jds/src/components/Input/InputArea/inputArea.styles.tspackages/jds/src/components/Input/SelectField/SelectFieldButton.tsxpackages/jds/src/components/Input/TagField/TagFieldButton.tsxpackages/jds/src/components/Input/TextField/TextFieldButton.tsxpackages/jds/src/components/Input/shared/FormField.tsxpackages/jds/src/components/Input/shared/field.styles.tspackages/jds/src/components/Label/Label.stories.tsxpackages/jds/src/components/Label/Label.style.tspackages/jds/src/components/Label/Label.tsxpackages/jds/src/components/Label/index.tspackages/jds/src/components/Label/label.types.tspackages/jds/src/components/Menu/MegaMenu/MegaMenu.tsxpackages/jds/src/components/Menu/MegaMenu/megaMenu.styles.tspackages/jds/src/components/Menu/Menu/Menu.stories.tsxpackages/jds/src/components/Menu/Menu/Menu.styles.tspackages/jds/src/components/Menu/Menu/Menu.tsxpackages/jds/src/components/Menu/Menu/menu.types.tspackages/jds/src/components/Menu/Menu/menu.variants.tspackages/jds/src/components/Menu/MenuItem/MenuItem.tsxpackages/jds/src/components/Menu/MenuItem/menuItem.styles.tspackages/jds/src/components/Radio/Radio.style.tspackages/jds/src/components/SegmentedControl/SegmentedControl.tsxpackages/jds/src/components/SegmentedControl/segmentedControl.styles.tspackages/jds/src/components/Select/Select.tsxpackages/jds/src/components/Select/SelectCheckbox.tsxpackages/jds/src/components/Select/SelectList.tsxpackages/jds/src/components/Select/SelectRadio.tsxpackages/jds/src/components/Select/select.styles.tspackages/jds/src/components/Snackbar/Snackbar.tsxpackages/jds/src/components/Snackbar/snackbar.styles.tspackages/jds/src/components/Step/Step.tsxpackages/jds/src/components/Step/step.styles.tspackages/jds/src/components/Tab/tab.styles.tspackages/jds/src/components/Tab/tab.tsxpackages/jds/src/components/Table/compound/Table.styles.tspackages/jds/src/components/Table/compound/TableHeaderItem.tsxpackages/jds/src/components/Table/compound/TableRowItem.tsxpackages/jds/src/components/Title/Title.stories.tsxpackages/jds/src/components/Title/Title.style.tspackages/jds/src/components/Title/Title.tsxpackages/jds/src/components/Title/index.tspackages/jds/src/components/Toast/Toast.tsxpackages/jds/src/components/Toast/toast.styles.tspackages/jds/src/components/Tooltip/Tooltip.stories.tsxpackages/jds/src/components/Uploader/Uploader.tsxpackages/jds/src/components/Uploader/UploaderFile.stories.tsxpackages/jds/src/components/Uploader/uploader.styles.tspackages/jds/src/components/index.tspackages/jds/src/tokens/Tokens.mdxpackages/jds/src/tokens/breakpoints.tspackages/jds/src/tokens/globalStyles.tspackages/jds/src/tokens/globalTokens.css.tspackages/jds/src/tokens/input/textStyle.jsonpackages/jds/src/tokens/input/textStyles.jsonpackages/jds/src/tokens/input/token.jsonpackages/jds/src/tokens/textStyles.css.tspackages/jds/src/tokens/theme.tspackages/jds/src/tokens/tokens.tspackages/jds/src/tokens/tokensBuilder.tspackages/jds/src/tokens/vars.css.tspackages/jds/src/utils/index.tspackages/jds/src/utils/typography.css.tspackages/jds/src/utils/typography.ts
💤 Files with no reviewable changes (15)
- packages/jds/src/components/Hero/Hero.stories.tsx
- packages/jds/src/components/Hero/Hero.tsx
- packages/jds/src/components/Label/index.ts
- packages/jds/src/components/Hero/index.ts
- packages/jds/src/components/Label/Label.stories.tsx
- packages/jds/src/components/Label/label.types.ts
- packages/jds/src/components/Title/Title.stories.tsx
- packages/jds/src/components/Title/Title.tsx
- packages/jds/src/tokens/input/textStyle.json
- packages/jds/src/components/Title/Title.style.ts
- packages/jds/src/components/Hero/Hero.style.ts
- packages/jds/src/components/Label/Label.style.ts
- packages/jds/src/components/Label/Label.tsx
- packages/jds/src/components/Title/index.ts
- packages/jds/src/components/index.ts
| htmlFor={inputId} | ||
| size='md' | ||
| weight='bold' | ||
| className={getLabelClassName({ weight: "bold" })} |
There was a problem hiding this comment.
라벨 타이포 크기 지정 누락으로 입력 컴포넌트 간 스타일 불일치가 발생할 수 있습니다.
Line 43은 weight만 지정되어 기본 size가 적용됩니다. 같은 입력 계열(InputArea, SelectField, TagField, FormField) 라벨은 size: "sm"를 사용 중이라 시각 차이가 생길 수 있습니다.
수정 제안
- className={getLabelClassName({ weight: "bold" })}
+ className={getLabelClassName({ size: "sm", weight: "bold" })}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/jds/src/components/Input/TextField/TextFieldButton.tsx` at line 43,
The label in TextFieldButton uses getLabelClassName({ weight: "bold" }) without
specifying size, causing inconsistent label sizing across
InputArea/SelectField/TagField/FormField which use size: "sm"; update the call
in TextFieldButton (where getLabelClassName is invoked) to include size: "sm"
alongside weight to match the other input components and ensure consistent label
typography.
Comment on lines
+60
to
+62
| className={getLabelClassName({ | ||
| size: size === "md" ? "sm" : "xs", | ||
| })} |
There was a problem hiding this comment.
캡션 타이포그래피 weight가 누락되었습니다.
캡션은 보조 텍스트인데 여기서는 weight: "subtle"이 빠져 기본 weight로 적용됩니다. SelectList의 캡션 처리와 맞추려면 subtle을 명시하는 게 안전합니다.
🔧 제안 수정안
<StyledSelectItemCaption
className={getLabelClassName({
size: size === "md" ? "sm" : "xs",
+ weight: "subtle",
})}
$isDisabled={isDisabled}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| className={getLabelClassName({ | |
| size: size === "md" ? "sm" : "xs", | |
| })} | |
| className={getLabelClassName({ | |
| size: size === "md" ? "sm" : "xs", | |
| weight: "subtle", | |
| })} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/jds/src/components/Select/SelectCheckbox.tsx` around lines 60 - 62,
The caption typography in SelectCheckbox is missing the subtle weight, so update
the getLabelClassName call in SelectCheckbox (where size is passed) to also
include weight: "subtle" so the caption uses the same subtle weight as
SelectList; modify the props passed to getLabelClassName accordingly to include
weight: "subtle" alongside the existing size logic.
itwillbeoptimal
approved these changes
May 23, 2026
Member
itwillbeoptimal
left a comment
itwillbeoptimal
left a comment
There was a problem hiding this comment.
고생하셨습니다~ 실제 사용 시 문제되는 부분은 없어 보이는데, 몇 가지 자잘한 코멘트 남겼습니다. 확인 부탁드립니다 🙂
Comment on lines
+21
to
+25
| const alignClassName: Record<HeroAlign, string> = { | ||
| center: "justify-center", | ||
| left: "justify-start", | ||
| right: "justify-end", | ||
| }; |
Member
There was a problem hiding this comment.
Title, Hero, Label 모두 textAlign이 현재 flex 정렬만 설정하고 실제 CSS text-align을 설정하지 않는데 의도된 동작인지 궁금합니다
Comment on lines
+34
to
+45
| export const titleStylesMap = { | ||
| "2xl": "semantic-textStyle-title-6", | ||
| xl: "semantic-textStyle-title-5", | ||
| lg: "semantic-textStyle-title-4", | ||
| md: "semantic-textStyle-title-3", | ||
| sm: "semantic-textStyle-title-2", | ||
| xs: "semantic-textStyle-title-1", | ||
| } as const satisfies Record<TitleSize, string>; | ||
|
|
||
| export const getLabelTokenKey = (size: LabelSize, weight: LabelWeight): string => { | ||
| return `semantic-textStyle-label-${size}-${weight}`; | ||
| }; |
Member
There was a problem hiding this comment.
(참고) 현재 두 부분 모두 실제 사용처는 없지만, getTitleClassName에서 titleStylesMap 대신 recipe를 직접 호출하고 있어 토큰 수정 시 두 곳을 각각 관리해야 할 가능성이 있어 보입니다~
Member
There was a problem hiding this comment.
두 값이 담고 있는 매핑은 이미 typography.css.ts의 recipe에 그대로 있어서 삭제해도 무방할 것 같네용
Comment on lines
+47
to
+58
| export const getLabelClassName = ({ | ||
| size = "md", | ||
| textAlign = "left", | ||
| weight = "normal", | ||
| cursor = "default", | ||
| }: LabelOwnProps = {}) => | ||
| typographyStyles.label({ | ||
| size, | ||
| textAlign, | ||
| weight, | ||
| cursor, | ||
| }); |
Member
There was a problem hiding this comment.
LabelOwnProps에 color 필드가 정의되어 있는데 이 함수에 전달되지 않아서 무시될 수 있을 것 같습니다
Member
Author
There was a problem hiding this comment.
해당 prop은 확장성을 고려하느라 넣어 뒀던 것 같은데요. 지금 다시 보니 color는 상위 래퍼에서 오버라이드하는 형식으로 사용하는 곳이 대부분이더라구요. 함수에 전달했을 때에 사용 빈도가 적거나 거의 없을 거라 예상되어 통일성을 유지하기 위해 아예 제거했습니다. 9590017
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/jds/src/utils/index.ts (1)
4-5: ⚡ Quick winutils/index.ts의 추가 export 범위가 PR 목적과도 연관되는지 점검
focusRing.css,overlay.css는Button/Radio/LabelButton등에서 실제로 사용되는 pseudo-element(focus/hover) 유틸로,PSEUDO_ELEMENT_POLICY.md와 함께 설계되어 있어 “typography 리팩토링과 무관”하다고 보기 어렵습니다.forwardRef도Image,CardOverlay등에서PolymorphicForwardRef로 사용되고 있어 유틸 export로서 자연스럽습니다.typography(getLabelClassName,getTitleClassName,shouldForwardTypographyProp,typographyobject)는 다수 컴포넌트/문서에서 이미 사용 중이라(예:Tab,Dialog,Select,Tokens.mdx) 공개 API 노출 방향은 타당해 보입니다.- 다만
index.ts가export *로 유틸 전부를 노출하므로(특히focusRing.css,overlay.css), PR 목표(“typography 통합”) 대비 “의도한 public surface”인지 범위만 최종 확인이 필요합니다. (index.ts 4-5, 8-9)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jds/src/utils/index.ts` around lines 4 - 5, The utils index currently exports everything including focusRing.css and overlay.css plus forwardRef and typography helpers (getLabelClassName, getTitleClassName, shouldForwardTypographyProp, typography); confirm whether exposing focusRing.css and overlay.css is intentional for this PR (typography consolidation) or if they should remain internal per PSEUDO_ELEMENT_POLICY, and if not intentional, narrow the exports in utils/index.ts to only the intended public symbols (e.g., keep forwardRef and the typography helpers but remove or move focusRing.css and overlay.css) or add explicit documentation/notes explaining the expanded public surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/jds/src/utils/index.ts`:
- Around line 4-5: The utils index currently exports everything including
focusRing.css and overlay.css plus forwardRef and typography helpers
(getLabelClassName, getTitleClassName, shouldForwardTypographyProp, typography);
confirm whether exposing focusRing.css and overlay.css is intentional for this
PR (typography consolidation) or if they should remain internal per
PSEUDO_ELEMENT_POLICY, and if not intentional, narrow the exports in
utils/index.ts to only the intended public symbols (e.g., keep forwardRef and
the typography helpers but remove or move focusRing.css and overlay.css) or add
explicit documentation/notes explaining the expanded public surface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69949169-e462-4933-a8e7-d96054c7c8b0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
packages/jds/package.jsonpackages/jds/src/components/Select/SelectRadio.tsxpackages/jds/src/utils/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/jds/package.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 작업 내용
jds패키지의Hero,Title,Label컴포넌트 제거 및apps/web로컬 컴포넌트 생성jds내부 제거 컴포넌트 사용처를 VE 기반 typography 유틸로 교체cn유틸리티 함수 추가 (clsx+tailwind-merge)💡 자세한 설명
파일 변경점이 많은데, 세 컴포넌트 제거에 대한 대응이 대부분이라 가볍게 보고 넘어가 주시면 됩니다!
1. 텍스트 스타일 토큰 생성 방식 개선
textStyle.json→textStyles.jsontoken,fontFamily,fontWeight,fontSize,lineHeight,letterSpacing만 포함하는 플랫 구조{ "textStyles": [ { "token": "semantic-textstyle-title-6", "type": "text", "fontFamily": "Pretendard Variable", "fontWeight": "600", "fontSize": "48px", "lineHeight": "62px", "letterSpacing": "-1.25px" }, ... ] }tokensBuilder.ts로직 추가:normalizeExtractedTextStyles함수 신규 구현token문자열(semantic-textstyle-body-md-normal)을 파싱해 category / size / weight를 추출primitive/font/size/{category}/{size}등)를 조합var(--typo-primitive-fontSize-*))로 연결textstyle)을 코드 관례(textStyle)로 정규화해 하위 호환 유지자동 생성 결과:
textStyles.css.ts,globalStyles.ts,theme.ts,tokens.ts에 누락됐던title-5,title-6토큰이 추가 생성됨관련하여
Tokens.mdx에 반영해 두었으니 참고 부탁드립니다.2-1.
jds패키지의Hero,Title,Label컴포넌트 제거기존에 세 컴포넌트를 사용하던 파일에 어떻게 적용할까 생각하다
textStyle이 여러 텍스트 관련 토큰을 한 번에 품고 있는 것이라, 중복 코드를 줄이기 위해서 유틸리티 함수가 필요하다고 판단했습니다.추가로 JDS가
vanilla-extract로 migration 중인 점을 반영해jds/src/utils/typography.ts도 Emotion 기반 구현에서 VE 기반 구현으로 변경했습니다.src/utils/typography.css.ts추가label,titlerecipe를 정의하고 기존semantic-textStyle-*전역 클래스를 조합src/utils/typography.ts변경styled,Theme,CSSObject의존성 제거getLabelClassName,getTitleClassName,typography유틸 추가src/utils/index.ts에서 typography 유틸을 export해 JDS utils entry에서 사용할 수 있도록 처리styled(Label),styled(Title), JSX의<Label>사용처를 실제 DOM 태그와getLabelClassName/getTitleClassName조합으로 교체LabelSize,LabelOwnProps타입 import를 제거된 component 경로가 아닌@/utils/typography로 교체Accordion,Badge,Dialog,EmptyState,FileItem,Input,Menu,Select,Snackbar,Step,Tab,Table,Toast,Uploader등 내부 컴포넌트의 typography 의존성을 함께 전환Hero,Title,Label컴포넌트는 JDS public export로 복구하지 않음2-2.
apps/web로컬 컴포넌트 생성이 과정에서 스타일을 부모에서 덧씌울 때
!(important)를 사용하는 케이스가 많이 생겨tailwind-merge를 설치하고cn유틸리티를 구현했습니다. 직접 관련한 유틸을 처음부터 구현하는 것보다 라이브러리를 설치해 사용하는 게 비용 측면에서 좋다고 생각했는데, 1팀 작업 관련이라 추후 제거나 지속 사용을 논의할 필요성이 있어 보입니다.cn유틸리티(clsx+tailwind-merge)로 조건부 클래스와 외부에서 전달된className를 병합apps/web/src/utils/cn.ts에clsx+tailwind-merge조합으로 구현Label은asprop을 지원해<label>,<span>,<p>등 다형성 렌더링 가능📗 참고 자료 (선택)
📢 리뷰 요구 사항 (선택)
sizeprop 네이밍이 기존jds컴포넌트와 일부 다릅니다. 사용처 전체를 일괄 수정했으나 누락이 없는지 확인 부탁드립니다.Label/Title컴포넌트 의존성을 className 유틸 조합으로 바꾸면서 시각적 차이가 생기지 않았는지 함께 확인 부탁드립니다.@jects/jds/utils에서 typography 유틸을 노출하는 방향이 적절한지 확인 부탁드립니다.✅ 셀프 체크리스트
closes #435
Summary by CodeRabbit
릴리스 노트
New Features
Refactor