refactor: 이미지 컴포넌트 리뉴얼#443
Conversation
- default는 thumbnail 컴포넌트 내 인터페이스로 소유하도록 변경
- 중요도가 높은 컴포넌트 일 경우에만 eager로 선언
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
itwillbeoptimal
left a comment
itwillbeoptimal
left a comment
There was a problem hiding this comment.
전체적으로 완성도 높게 잘 구현되어 있는 것 같습니다
단순 제안 정도로만 코멘트 남겨두었습니다! 고생하셨습니다 🙂
| <Comp | ||
| ref={forwardedRef as Ref<HTMLDivElement>} | ||
| {...restProps} | ||
| data-part='root' | ||
| className={clsx( | ||
| thumbnailStyles.root({ ratio, orientation, cornerStyle, appearance }), | ||
| className, | ||
| )} | ||
| > |
There was a problem hiding this comment.
폴백 대체 시 alt가 붙지 않아서 isFallbackVisible 분기에 따라 role, aria-label 같은 접근성 속성을 추가해도 좋을 것 같아요
|
|
||
| return ( | ||
| <Comp | ||
| ref={forwardedRef as Ref<HTMLDivElement>} |
There was a problem hiding this comment.
(참고) 동작 상 문제되지 않는 부분인데, 코드를 보면서 공개 타입은 HTMLElement인데 내부에서 HTMLDivElement로 좁혀져 있는 것이 의아해서 as Ref<HTMLElement>로 변경해 보니 TS 에러가 발생하더라구요!
지금처럼 캐스트를 유지하거나 아래처럼 분기해서 처리하는 방법도 있을 것 같아용
const sharedProps = {
"data-part": "root" as const,
className: clsx(
thumbnailStyles.root({ ratio, orientation, cornerStyle, appearance }),
className,
),
...restProps,
};
if (asChild) {
return (
<Slot.Root ref={forwardedRef} {...sharedProps}>
<Slot.Slottable>{children}</Slot.Slottable>
{content}
</Slot.Root>
);
}
return (
<div ref={forwardedRef as Ref<HTMLDivElement>} {...sharedProps}>
{content}
</div>
);
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
위에서 작성된 타입을 아래처럼 수정하면 될 것 같은데,
HTMLElement가 아니라HTMLDivElement로 변경하는 건 어떤가요?export const Thumbnail = forwardRef<HTMLDivElement, ThumbnailProps>(
asChild = false만 고려하면 HTMLDivElement로 두는 것도 괜찮지만, asChild = true인 경우에는 실제로 전달한 자식 요소(button, a 등)를 ref가 가리키게 될 수 있어서, HTMLDivElement로 타입을 지정하면 타입 정보와 실제 값이 달라질 여지가 있을 것 같아요! 그래서 HTMLElement가 조금 더 적절한 타입이지 않을까 생각합니다
- 폴백 표시 시 root에 role="img", aria-label을 부여해 스크린리더 대응 - asChild(Slot)/div 경로를 분리해 잘못된 ref 캐스트를 제거 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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:
WalkthroughImage를 대체하는 새 Thumbnail 컴포넌트를 추가(타입·스타일·핵심 구현·폴백·스토리)하고, Banner/Card/MenuItem 등에서 Image 사용을 Thumbnail로 마이그레이션하며 Image에는 deprecated 표시를 추가합니다. ChangesThumbnail 도입 및 Image 마이그레이션
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jds/src/components/Banner/Banner.tsx (1)
62-76:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
style스프레드 순서로 인한 포지셔닝 손실 가능성
{...imgProps}가 명시적style보다 뒤에 전개되어, 소비자가style을 전달하면 absolute 포지셔닝 전체가 덮어써집니다.BannerImageProps는style을 제외하지 않으므로 타입상 전달 가능하며, 이 경우 배너 오버레이 레이아웃이 깨질 수 있습니다.🛠️ 제안 수정 (style 병합)
<Thumbnail ratio='9:16' orientation='landscape' appearance='hollow' alt={title} + {...imgProps} style={{ position: "absolute", top: 0, left: 0, width: "100%", height: "100%", zIndex: 0, + ...imgProps.style, }} - {...imgProps} />🤖 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/Banner/Banner.tsx` around lines 62 - 76, The Thumbnail call currently spreads {...imgProps} after a hardcoded style, so any style in BannerImageProps/imgProps will overwrite required positioning; fix by merging styles instead of letting imgProps.style win — create a mergedStyle (e.g. combine imgProps.style with the absolute positioning defaults, ensuring defaults take precedence) and pass that mergedStyle to Thumbnail while still spreading imgProps (or spread imgProps first and then pass mergedStyle), referencing the Thumbnail component, the imgProps/BannerImageProps object, and the style prop so the absolute top/left/width/height/zIndex aren’t lost.
♻️ Duplicate comments (1)
packages/jds/src/components/Thumbnail/Thumbnail.tsx (1)
64-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
asChild경로의 fallback 상태에서는 이름 없는 버튼/링크가 될 수 있습니다.이전 피드백과 같은 맥락인데, 현재 보완은
div분기에만 들어가 있어서asChild에서는 이미지가 깨지는 순간<img alt>가 사라지고 아이콘만 남습니다. 자식이 별도 라벨을 주지 않은 경우 접근 가능한 이름이 없어지니, fallback 노출 시에는 이 경로에도 최소한aria-label={alt}를 같이 전달하는 게 좋겠습니다.제안 수정
+ const fallbackA11yProps = isFallbackVisible ? { "aria-label": alt } : {}; + if (asChild) { return ( - <Slot.Root ref={forwardedRef} {...sharedProps} {...restProps}> + <Slot.Root ref={forwardedRef} {...sharedProps} {...fallbackA11yProps} {...restProps}> <Slot.Slottable>{children}</Slot.Slottable> {content} </Slot.Root> ); }🤖 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/Thumbnail/Thumbnail.tsx` around lines 64 - 69, When rendering the asChild branch inside Thumbnail (the Slot.Root wrapper used with forwardedRef, sharedProps, restProps, children and content), ensure the Slot.Root receives an accessible name by adding aria-label={alt} (or aria-label={props.alt}) so that if children don’t provide a label or the image falls back, the control remains accessible; implement this in the asChild return path of the Thumbnail component (where Slot.Root is created) so Slot.Root gets aria-label when alt is present.
🤖 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/Card/compound/CardImage.tsx`:
- Around line 71-91: The badge span in CardImage uses aria-hidden='true' which
hides meaningful text (badgeLabel) from assistive tech; locate the badge
rendering in CardImage (references: badgeVisible, badgeLabel) and either remove
aria-hidden or provide an accessible alternative (e.g., keep the visual span but
add an offscreen/readable element or aria-label on the container that exposes
badgeLabel) so screen readers receive the badge text while preserving visual
styling.
- Around line 74-87: The badge uses absolute positioning relative to the wrong
ancestor and hard-coded styles; update StyledCardImageContainer to include
position: "relative" so the badge anchors to the image container (rather than
StyledCardRoot), and replace the hard-coded badge values in CardImage.tsx
(backgroundColor, color, top, left, padding, fontSize, lineHeight, borderRadius)
with the appropriate card CSS variables/tokens (e.g., --card-caption-color,
--card-label-color or new --card-badge-bg / --card-badge-color /
--card-badge-padding / --card-badge-font-size) so spacing and colors follow the
card tokens and theme.
In `@packages/jds/src/components/Thumbnail/thumbnail.css.ts`:
- Around line 75-79: 현재 thumbnail.css.ts의 base 스타일(현재 display block에 설정된
overflow: hidden)과 다른 블록(약 96-104, 132-135 범위)에서 포커스 링을 ::before의 밖쪽 box-shadow로
그리고 있어, overflow: hidden 때문에 키보드 포커스 링이 잘려 보이지 않습니다; asChild로 전달되는 인터랙티브 경로에서
접근성 차단이 되므로, base 스타일에서 overflow: hidden을 제거하거나 포커스 링을 외부 그림자 대신 내부(inset)로 바꿔
::before/::after 또는 focus-visible 상태에서 inset box-shadow 또는 내부 outline으로 렌더링하도록
수정하세요; 구체적으로는 thumbnail.css.ts의 base 블록에서 overflow 속성 조정하고, ::before 기반 외부
box-shadow 코드를 찾아(또는 관련 focus selector를 찾아) 이를 inset box-shadow 또는 focus-visible
{ outline: 2px solid ...; outline-offset: -2px } 형태로 바꾸어 포커스 링이 요소 내부에 표시되게 하세요.
In `@packages/jds/src/components/Thumbnail/thumbnail.types.ts`:
- Around line 36-38: The ThumbnailContentProps union should narrow the children
type to a single ReactElement when asChild is true to match
Slot.Slottable/Slot.cloneElement contract; update the type in thumbnail.types.ts
so the branch { asChild: true; children: ReactElement } (import ReactElement
from react) replaces ReactNode, keep the { asChild?: false; children?: never }
branch as-is, and verify Thumbnail.tsx (the component that wraps children in
<Slot.Slottable>) compiles with the tightened type.
---
Outside diff comments:
In `@packages/jds/src/components/Banner/Banner.tsx`:
- Around line 62-76: The Thumbnail call currently spreads {...imgProps} after a
hardcoded style, so any style in BannerImageProps/imgProps will overwrite
required positioning; fix by merging styles instead of letting imgProps.style
win — create a mergedStyle (e.g. combine imgProps.style with the absolute
positioning defaults, ensuring defaults take precedence) and pass that
mergedStyle to Thumbnail while still spreading imgProps (or spread imgProps
first and then pass mergedStyle), referencing the Thumbnail component, the
imgProps/BannerImageProps object, and the style prop so the absolute
top/left/width/height/zIndex aren’t lost.
---
Duplicate comments:
In `@packages/jds/src/components/Thumbnail/Thumbnail.tsx`:
- Around line 64-69: When rendering the asChild branch inside Thumbnail (the
Slot.Root wrapper used with forwardedRef, sharedProps, restProps, children and
content), ensure the Slot.Root receives an accessible name by adding
aria-label={alt} (or aria-label={props.alt}) so that if children don’t provide a
label or the image falls back, the control remains accessible; implement this in
the asChild return path of the Thumbnail component (where Slot.Root is created)
so Slot.Root gets aria-label when alt is present.
🪄 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: 516c2b8a-6076-4995-9a9f-1d85285e9548
📒 Files selected for processing (19)
packages/jds/src/components/Banner/Banner.tsxpackages/jds/src/components/Banner/banner.types.tspackages/jds/src/components/Card/Card.types.tspackages/jds/src/components/Card/compound/CardImage.tsxpackages/jds/src/components/Card/presets/PlateWithLabel.tsxpackages/jds/src/components/Card/presets/PlateWithTitle.tsxpackages/jds/src/components/Image/Image.stories.tsxpackages/jds/src/components/Image/Image.style.tspackages/jds/src/components/Image/Image.tsxpackages/jds/src/components/Image/index.tspackages/jds/src/components/Menu/MenuItem/MenuItem.tsxpackages/jds/src/components/Menu/MenuItem/menuItem.styles.tspackages/jds/src/components/Thumbnail/Thumbnail.stories.tsxpackages/jds/src/components/Thumbnail/Thumbnail.tsxpackages/jds/src/components/Thumbnail/ThumbnailFallback.tsxpackages/jds/src/components/Thumbnail/index.tspackages/jds/src/components/Thumbnail/thumbnail.css.tspackages/jds/src/components/Thumbnail/thumbnail.types.tspackages/jds/src/components/index.ts
💤 Files with no reviewable changes (4)
- packages/jds/src/components/Image/index.ts
- packages/jds/src/components/Image/Image.stories.tsx
- packages/jds/src/components/Image/Image.style.ts
- packages/jds/src/components/Image/Image.tsx
- 포커스 링을 inset box-shadow로 변경해 overflow:hidden에 잘리지 않도록 수정 - asChild children 타입을 ReactElement로 좁혀 Slot clone 계약과 일치 - Card 뱃지의 불필요한 aria-hidden 제거 및 컨테이너 position:relative 기준 추가 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Zero-1016
left a comment
Zero-1016
left a comment
There was a problem hiding this comment.
Image 컴포넌트 deprecated 처리 및, Thumbnail 컴포넌트 추가 확인했어요.
처음 리뷰를 하는 만큼 아무래도 구조라든지 코드 작성 방법에 대한 질문과 간단한 개선사항이 좀 있어요. 가볍게 봐주세요.
| @@ -41,32 +41,53 @@ export const CardImage = forwardRef<HTMLDivElement, CardImageProps>( | |||
| }; | |||
There was a problem hiding this comment.
객체의 value 마다 as const를 붙이신 이유가 있을까요? 다음 처럼 처리할 수 있어보여요.
const defaultRatioMap = {
plate: {
vertical: "2:3",
vertical: "3:4",
horizontal: "1:1",
},
post: {
vertical: "1:2",
horizontal: "1:1",
},
} as const;There was a problem hiding this comment.
추가로 속성 값을 가져오는 로직이면, 순수 함수 작성하여 컴포넌트 외부로 분리해도 좋을 것 같아요.
컴포넌트 코드에는 그려지는 로직만 있어서 가독성이 높아질 것 같아요.
There was a problem hiding this comment.
맞아요 요 부분은 레거시 쪽이라 Card쪽이 compound 쪽으로 되어있는거 같아서 같이 수정된 부분인데 요 부분은 신경 안쓰셔도 될 거같아요! (@deprecated 처리만 해놓겠습니당)
| interface ThumbnailBaseProps extends ComponentPropsWithoutRef<"div"> { | ||
| src?: string; | ||
| alt: string; | ||
| loading?: "lazy" | "eager"; |
There was a problem hiding this comment.
Thumbnail이면 <img /> 태그가 더 어울리지 않을까요? 그럼 src나, alt, loading의 경우 지금과 다르게 타입이 관리가 될 수 있을 것 같고.
추후에도 사용할 때 img 태그를 생각하면서 사용할 수 있을 것 같은데요. div 태그를 base로 잡으신 이유가 있으실까요?
| interface ThumbnailBaseProps extends ComponentPropsWithoutRef<"div"> { | |
| src?: string; | |
| alt: string; | |
| loading?: "lazy" | "eager"; | |
| interface ThumbnailBaseProps extends ComponentPropsWithoutRef<"img"> { |
There was a problem hiding this comment.
img가 의미상 더 자연스럽다는 점은 동의합니다~
다만 이 컴포넌트는 이미지 로드 실패 시 폴백을 렌더링하는 기능을 제공하는데, 이를 위해 폴백을 자식으로 렌더링할 수 있는 컨테이너 엘리먼트가 필요합니다. img는 void element라 자식을 가질 수 없어서 현재와 같은 방식으로 폴백을 제공하기는 어려워 보여요
그리고 Storybook 문서를 보면 caller가 root element의 의미, 동작, a11y를 결정하고 Thumbnail은 시각적 표현만 책임진다는 방향으로 설계된 것 같습니다. 이런 관점에서 div는 의미를 갖지 않는 컨테이너 역할만 수행하고, 다양한 컨텍스트에서 소비자가 필요한 시맨틱을 직접 선택할 수 있을 것 같습니다
There was a problem hiding this comment.
해당 부분이 가장 많이 고민했던 부분인데 의미론적으로 <img> 가 맞다고 생각해요. (기존 Image도 이미지 태그 베이스로 구성되어있었어요)
<div/> 를 사용하게 된 계기가 기존에 polymorphic 유틸리티를 제거하고 slot형태로 처리하면서 발생하게된 side effect인데 Thumbnail을 anchor나 div(순수 display) 로 제공해야할 때에도 img 태그 base로 가지는게 적절하다고 생각하시는지 질문드려요!
There was a problem hiding this comment.
아하, 두 분 얘기 들으니 완벽하게 이해했습니다.
API가 기존 <img />와 워낙 유사하다 보니, div prop 타입 위에 src·alt 같은 img 속성을 얹어가며 작성하는 게 왜 필요한지에서 시작된 의문이었어요.
Thumbnail을 anchor나 div(순수 display)로 제공해야 할 때에도 img 태그 base로 가지는 게 적절한지
asChild가 있으니 지금 이대로 유지해도 될 것 같아요. <a><img/></a>, <div><img/></div> 전부 가능하니까요. img를 root가 아니라 content에 두는 구조라고 이해하면 깔끔하네요.
| style={{ | ||
| position: "absolute", | ||
| top: "8px", | ||
| left: "8px", | ||
| zIndex: 1, | ||
| minWidth: "18px", | ||
| padding: "0 6px", | ||
| borderRadius: "2px", | ||
| backgroundColor: "rgba(0, 0, 0, 0.6)", | ||
| color: "#fff", | ||
| fontSize: "12px", | ||
| lineHeight: "18px", | ||
| textAlign: "center", | ||
| }} |
There was a problem hiding this comment.
이 코드에 직접적인 질문은 아닌데요.
혹시 인라인 스타일, 또는 **.style.ts or **.css.ts 에 정의하는 기준이 있을까요?
There was a problem hiding this comment.
이 부분은 추후에 .claude 와 같은 정책서가 있으면 좋을 거 같아요. (공통적으로 스타일을 맞추기 위함)
해당 부분 역시 레거시인데 현재 스타일링 이관 작업이기 때문에 내부 inline, 혹은 style.ts 로 표현된 부분들을 모두 외부 css.ts로 표현 중인 작업을 진행중입니다.
++)CardImage 역시 레거시인데 충돌 방지로 임시 수정된 부분임을 노티드려요.
There was a problem hiding this comment.
아하, 혹시 기존에 관리되던 작성 규칙 같은 파일이 있을까요? 아니면 각자 센스 것..? 작업을 했는지.
dev 브랜치에 생각보다 많은 것들이 한번에 이뤄지고 있는 것 같아서, 나중에 한번에 다운로드 해볼게요~! 감사합니다.
There was a problem hiding this comment.
작성 규칙에 관해서라면 FE Archive 컨벤션 문서가 있긴 한데요. 초기에 작성된 거라 원준 님이 말씀해 주신 것처럼 정책서를 새롭게 생성해야 할 것으로 보여요. (.AGENTS, .claude)
2팀이 생성되고 작업이 넘어오면서, 디자인 시스템을 관리하던 인원의 변동이 있었다 보니 네이밍이나 스타일 면에서 다른 것들이 다수 존재하는 상황이에요. 질문 주신 것처럼 정의하는 기준이 별도로 존재하지 않고, 눈치껏 작업하던 걸 현재와 같이 리뷰로 보완하던 방식이 맞습니다.
|
|
||
| return ( | ||
| <Comp | ||
| ref={forwardedRef as Ref<HTMLDivElement>} |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| export * from "./Footer"; | ||
| export * from "./Hero"; | ||
| export * from "./Icon"; | ||
| /** @deprecated `Image`는 `Thumbnail`로 대체되었습니다. 하위 호환을 위해 한시적으로 유지합니다. */ |
There was a problem hiding this comment.
Image 컴포넌트에 deprecated가 잘 들어가 있어서 제거해도 될 듯 합니다!
| /** @deprecated `Image`는 `Thumbnail`로 대체되었습니다. 하위 호환을 위해 한시적으로 유지합니다. */ |
itwillbeoptimal
left a comment
itwillbeoptimal
left a comment
There was a problem hiding this comment.
갑자기 생각난 부분이라 같이 남겨봅니다!
현재 Thumbnail이 내부에서 <img>를 직접 렌더링하고 있어서, 소비 측 앱에서 next/image로 교체할 방법이 없는 상태로 보입니다
JDS 마이그레이션 목적 중 하나가 추후 Next.js 환경에서도 동일하게 활용할 수 있도록 하는 것이라면, 이미지 렌더링 방식을 소비자가 제어할 수 있는 구조를 열어두는 것도 고려할 수 있을 것 같습니다
renderImage?: (props: {
src: string;
alt: string;
className: string;
onError: () => void;
loading: "lazy" | "eager";
}) => ReactNode;이런 식으로 제공하면 기본 구현은 <img>를 유지하면서 Next.js 환경에서는 next/image를 주입해 사용할 수 있을 것 같아요~
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/jds/src/components/Thumbnail/Thumbnail.tsx (1)
18-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
asChild경로와 ref 공개 타입이 불일치합니다.
asChild=true에서는 실제 ref 대상이button/a가 될 수 있는데, 현재 공개 타입이HTMLDivElement라서 타입 계약이 깨집니다.forwardRef<HTMLElement, ThumbnailProps>로 되돌리거나(asChild 분기 유지), 폴리모픽 ref 타입으로 분리하는 편이 안전합니다.제안 수정안
-import { forwardRef, useState } from "react"; +import { forwardRef, useState, type Ref } from "react"; -export const Thumbnail = forwardRef<HTMLDivElement, ThumbnailProps>( +export const Thumbnail = forwardRef<HTMLElement, ThumbnailProps>( ... - return ( - <div - ref={forwardedRef} + return ( + <div + ref={forwardedRef as Ref<HTMLDivElement>} {...sharedProps} {...(isFallbackVisible && { role: "img", "aria-label": alt })} {...restProps} >#!/bin/bash # asChild 사용 위치에서 실제 자식 엘리먼트 타입(button/a/div 등) 확인 rg -n -C2 '<Thumbnail[^>]*asChild|<Slot\.Slottable>|<button|<a ' --type=tsx # Thumbnail ref 타입 선언 및 전달 경로 확인 rg -n -C3 'forwardRef<|ref=\{forwardedRef\}|Slot\.Root' packages/jds/src/components/Thumbnail/Thumbnail.tsx🤖 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/Thumbnail/Thumbnail.tsx` around lines 18 - 70, The public ref type for Thumbnail is incorrect: Thumbnail is declared with forwardRef<HTMLDivElement, ThumbnailProps> but when asChild is true the forwardedRef actually points to the child element (Slot.Root → whatever child like button/a), breaking the type contract; change the forwardRef generic to a broader type (e.g., forwardRef<HTMLElement, ThumbnailProps>) or implement a polymorphic ref type so forwardedRef matches actual DOM element types, and ensure all uses of forwardedRef (Slot.Root, the div branch) accept the updated ref type; update Thumbnail's declaration (forwardRef), ThumbnailProps if necessary, and verify references to Slot.Root and forwardedRef compile with the new type.
🤖 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.
Duplicate comments:
In `@packages/jds/src/components/Thumbnail/Thumbnail.tsx`:
- Around line 18-70: The public ref type for Thumbnail is incorrect: Thumbnail
is declared with forwardRef<HTMLDivElement, ThumbnailProps> but when asChild is
true the forwardedRef actually points to the child element (Slot.Root → whatever
child like button/a), breaking the type contract; change the forwardRef generic
to a broader type (e.g., forwardRef<HTMLElement, ThumbnailProps>) or implement a
polymorphic ref type so forwardedRef matches actual DOM element types, and
ensure all uses of forwardedRef (Slot.Root, the div branch) accept the updated
ref type; update Thumbnail's declaration (forwardRef), ThumbnailProps if
necessary, and verify references to Slot.Root and forwardedRef compile with the
new type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b728d5e-4f3b-43a4-b4bc-4826058fb93d
📒 Files selected for processing (3)
packages/jds/src/components/Card/compound/CardImage.tsxpackages/jds/src/components/Thumbnail/Thumbnail.tsxpackages/jds/src/components/Thumbnail/thumbnail.css.ts
💤 Files with no reviewable changes (1)
- packages/jds/src/components/Thumbnail/thumbnail.css.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jds/src/components/Card/compound/CardImage.tsx
Card 컴파운드 재구성 시 제거 예정인 CardImage에 @deprecated를 명시하고 Thumbnail 사용을 안내합니다. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WonJuneKim
force-pushed
the
refactor/431-image-renewal
branch
from
3b357ce to
c6afd18
Compare
renderImage 형태도 좋아 보이는데, render prop 대신 children을 콜백(children-as-function)으로 받는 방식은 어떨까요? 사실 저는 render* 같은 패턴을 선호하지 않는 편인데, 이름은 다 비슷한데(renderImage, renderItem, renderHeader…) 정작 넘겨야 하는 인자나 사용법은 제각각이라 매번 시그니처를 확인해야 하는 점이 아쉽더라고요. children 콜백으로 통일하면 “함수를 넘긴다”는 사용법이 일관되게 유지되는 장점도 있을 것 같아요. 그리고 children을 넘기지 않았을 때는 기본 |
// 기본 - children 안 넘기면 <img> 사용
<Thumbnail src="/photo.jpg" alt="사진" />
// 커스텀 - Next.js 환경에서 next/image 주입
<Thumbnail src="/photo.jpg" alt="사진">
{({ src, alt, className, onError, loading }) => (
<Image
src={src}
alt={alt}
className={className}
onError={onError}
loading={loading}
width={200}
height={200}
/>
)}
</Thumbnail> |
|
@Zero-1016 저도 수정해보면서 느낀점이 renderImage로 계약을 체결하게되면 부가적으로 알아야할 정보가 늘어나는 느낌이라 불편할 수 있겠다는 생각이 들었어요. 말씀해주신 children을 콜백하는 방식이 renderProps 방식보다 사용처에서 매끄럽게 사용될 수 있을 거 같네요 :) 좋은 의견 감사합니다! |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WonJuneKim
force-pushed
the
refactor/431-image-renewal
branch
from
41f1c2d to
b2f06b6
Compare
순수 <img> 기본값은 유지하고, 이미지 렌더링에 대한 DS의 대응 범위는 추후 논의가 필요하다는 점을 TODO로 표기합니다. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@Zero-1016 @itwillbeoptimal 두분이 주신 의견들을 취합해본 결과 renderProps를 제거하고 기존의 순수한 image 컴포넌트로 롤백처리했어요. 이유는 다음과 같아요. @Zero-1016 님이 제안해주신 방법은 소비자 입장에서 간편하게 사용할 수 있다는 장점이 있지만 children이 function이 되는 순간 현재 @itwillbeoptimal 말씀해주신 전제인 "Next.js 환경에서도 문제 없이 사용할 수 있는 DS를 만들자" 는 것이 이번 마이그레이션의 주요 목표였어요. 다만 말씀해주신 next/image의 경우 해당 컴포넌트 자체가 복합적인 책임을 가지고 있는 컴포넌트라고 생각해요. 자체 최적화 기능이나 특정 프레임 워크(Next)에 결합되어있는 것이 그 이유라고 생각합니다. 따라서 DS가 오히려 범용적이고 순수한 단위의 컴포넌트로써 보여져야한다고 생각하기 때문에 다시 기존의 presentation 역할만을 수행하는 API만 남겨놓았습니다. Next까지 고려한다면 Provider 설정을 추가할 수도 있었지만 이 부분 역시 소비자에게 불편함을 주는 부수효과라고 생각합니다. 이 부분은 TODO로만 남겨놓을게요. DS가 어디까지 범위를 보장해줄 것인가 해석하기에 따라 구현이 달라질 수 있는 부분인 거 같아요. |
WonJuneKim
force-pushed
the
refactor/431-image-renewal
branch
from
958613e to
443ec53
Compare
…ewal # Conflicts: # packages/jds/src/components/Menu/MenuItem/menuItem.styles.ts
4 participants
💡 작업 내용
💡 자세한 설명
✅ Thumbnail
✅ discriminated union
cornerStyle="rounded" ⇔ ratio="1:1" 에서만
orientation="landscape" ⇔ ratio≠"1:1" 에서만
children ⇔ asChild=true 에서만 (asChild면 children 필수 = Slot clone 대상)
✅ 스타일 토큰 관련
📗 참고 자료 (선택)
📢 리뷰 요구 사항 (선택)
✅ 셀프 체크리스트
closes #431
Summary by CodeRabbit
새로운 기능
지원 중단
변경 사항