feat(image): add image component#262
Conversation
minjeeki
requested review from
3o14,
G-hoon,
KYBee,
Yeom-JinHo,
froggy1014,
haedawn,
minji0214 and
osohyun0224
as code owners
🦋 Changeset detectedLatest commit: ac2eb99 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
| const sizeStyle: CSSProperties = { | ||
| width: typeof width === 'number' ? `${width}px` : width, | ||
| height: typeof height === 'number' ? `${height}px` : height, | ||
| objectFit: fit, | ||
| }; | ||
|
|
||
| const fillStyle: CSSProperties = fill | ||
| ? { | ||
| position: 'absolute', | ||
| inset: 0, | ||
| width: '100%', | ||
| height: '100%', | ||
| } | ||
| : {}; |
There was a problem hiding this comment.
다른 컴포넌트들과 마찬가지로 vanilla-extract로 스타일링하면 좋을꺼 같습니다 : )
| placeholder?: ReactNode; | ||
| } | ||
|
|
||
| export function Image({ |
There was a problem hiding this comment.
다른 컴포넌트와 마찬가지기로 forwardRef로 export하면 좋을꺼 같습니다
| onError: onErrorFromProps, | ||
| }: UseImageStatusParams): UseImageStatusResult { | ||
| const [status, setStatus] = useState<ImageStatus>('loading'); | ||
| const [imgSrc, setImgSrc] = useState(src); |
There was a problem hiding this comment.
src prop가 변경되면 갱신되어야 하는 로직 고민이 피요할꺼 같아요 🤔
There was a problem hiding this comment.
아... 민지님 이건 안됩니다...
function Gallery() {
const [currentSrc, setCurrentSrc] = useState('/photo-1.jpg');
return (
<>
<Image src={currentSrc} alt="gallery" width={400} height={300} />
<button onClick={() => setCurrentSrc('/photo-2.jpg')}>다음 사진</button>
</>
);
}
이런 src 가 부모에서 교체되는 상황이라면 이미지 교체가 안돼요.
src 가 바뀌면 imgSrc 가 업데이트 되게끔 되어야 합니다.
| "./styles.css": "./dist/index.css" | ||
| } | ||
| }, | ||
| "sideEffects": [ | ||
| "**/*.css" | ||
| ] |
There was a problem hiding this comment.
지금 따로 css나 vanilla-extract가 없어서 여기는 불필요할꺼 같습니다 : )
css none으로 가신다면 수정 필요한 부분입니당
|
많이 고민하신 부분이 보여서 너무 좋네요!
|
KYBee
left a comment
KYBee
left a comment
There was a problem hiding this comment.
Image 컴포넌트 작업 잘 봤습니다. 고생 많으셨습니다 :)
[nit] side export에는 아직 추가되지 않은 게 맞을까요?
| import { Image } from './Image'; | ||
|
|
||
| describe('Image', () => { | ||
| it('renders with required src and alt props', () => { |
G-hoon
left a comment
G-hoon
left a comment
There was a problem hiding this comment.
민지님 changeset 추가 부탁드립니다.
| onError: onErrorFromProps, | ||
| }: UseImageStatusParams): UseImageStatusResult { | ||
| const [status, setStatus] = useState<ImageStatus>('loading'); | ||
| const [imgSrc, setImgSrc] = useState(src); |
There was a problem hiding this comment.
아... 민지님 이건 안됩니다...
function Gallery() {
const [currentSrc, setCurrentSrc] = useState('/photo-1.jpg');
return (
<>
<Image src={currentSrc} alt="gallery" width={400} height={300} />
<button onClick={() => setCurrentSrc('/photo-2.jpg')}>다음 사진</button>
</>
);
}
이런 src 가 부모에서 교체되는 상황이라면 이미지 교체가 안돼요.
src 가 바뀌면 imgSrc 가 업데이트 되게끔 되어야 합니다.
|
|
||
| if (!hasTriedFallback && fallbackSrc) { | ||
| setImgSrc(fallbackSrc); | ||
| setHasTriedFallback(true); |
There was a problem hiding this comment.
제 생각엔, hasTriedFallback 상태를 제거해도 될 것 같아요.
검토 부탁드립니다.
if (status !== 'fallback' && fallbackSrc) {
setImgSrc(fallbackSrc);
setStatus('fallback');
return;
}
setStatus('error');
G-hoon
left a comment
G-hoon
left a comment
There was a problem hiding this comment.
다른 컴포넌트는 forwardRef 쓰고 있는데,
forwardRef 적용하는 것 검토해주세요~!
ref로 DOM에 접근해야 하는 경우(IntersectionObserver 등)도 있을 것 같습니다.
There was a problem hiding this comment.
이 file이 혹시 필요한가요...?
필요한 지 검토 부탁드립니다.
| @@ -0,0 +1,2 @@ | |||
| export * from './hooks/useImageStatus'; | |||
There was a problem hiding this comment.
이거 굳이 export 할 필요가 있을까요?
사용자 입장에서, 이 hook 을 사용할 일이 있을까 해서요
| type ImageFit = 'contain' | 'cover' | 'fill'; | ||
|
|
||
| export interface ImageProps | ||
| extends Omit<ComponentPropsWithoutRef<'img'>, 'src' | 'alt' | 'width' | 'height' | 'onLoad'> { |
There was a problem hiding this comment.
여기 onError 도 추가해주면 좋을 것 같습니다!
| @@ -0,0 +1 @@ | |||
| declare module '*.module.css'; | |||
There was a problem hiding this comment.
아마 이거 사용하실일 없을거예요. 이거 Template으로 만드신 패키지같은데, vanilla extract 마이그레이션 이전의 상태인가보네요.
이건 module.css 사용할때 있었던 잔재같아요 ~
| @@ -0,0 +1,57 @@ | |||
| { | |||
| "name": "@sipe-team/image", | |||
There was a problem hiding this comment.
side 패키지에도 추가가 되어야할거같아요.
| expect(img).toHaveAttribute('src', 'https://picsum.photos/400/300'); | ||
| }); | ||
|
|
||
| it('applies width and height styles for number and string values', () => { |
There was a problem hiding this comment.
이 컴포넌트를 next/image 대체로 쓰는 그림은 아니라기보다, native <img />를 감싼 디자인 시스템용 wrapper로 이해했습니다.
그래서 컴포넌트 자체보다 useImageStatus를 범용적으로 재사용할 수 있게 열어두는 쪽이 더 중요해 보여요. next/image 같은 다른 이미지 렌더러에서도 같은 loading/fallback DX를 만들 수 있도록 하고, 그 사용 시나리오를 테스트로 함께 보강하면 좋겠습니다.
- Sync imgSrc and loading state when src prop changes - Remove hasTriedFallback; use fallback status for placeholder visibility - Migrate styles to Vanilla Extract (fill/fit/hidden, sized + inline vars) - Use shared tsup config; remove obsolete global.d.ts - Add forwardRef to the underlying img element - Tighten onError typing (omit from rest, explicit handler type) - Stop exporting useImageStatus from the main package entry - Align @sipe-team/side aggregate exports with @sipe-team/image BREAKING CHANGE: useImageStatus is no longer exported from @sipe-team/image main entry
Changes올려주신 PR 코멘트 내용을 기반으로 코드를 수정했습니다. 상세 내용은 아래와 같습니다. 1. vanilla-extract로 스타일링 방식 통일관련 리뷰수정 사항
2. forwardRef 추가관련 리뷰수정사항
3. useImageStatus src 동기화 문제관련 리뷰수정사항
4. hasTriedFallback관련 리뷰수정사항
5. 그 외 작업
추가 변경 가능성
|
미해결 PR 리뷰useImageState 훅 활용 관련관련 리뷰문제 파악
제안하는 방향@sipe-team/image/hooks와 같이 서브패스를 제공하는 것은 어떨지 제안해봅니다.
작업 진행 계획
|
5 participants
Changes
@sipe-team/image 패키지의 Image 컴포넌트 초기 구현입니다.
Image 컴포넌트 기본 동작 구현
useImageStatus 훅 분리
에러/대체 이미지 상태 관리를 위해 useImageStatus 훅을 추가했습니다.
Storybook Components/Image 추가
Image 테스트 코드 추가
총 8개 케이스를 작성했습니다 (
packages/image/src/Image.test.tsx)Visuals
WithPlaceholder — 로딩 중 placeholder
https://github.com/user-attachments/assets/10505675-28da-4180-9137-07aa02207c49
WithFill — fill 동작
Checklist
Additional Discussion Points
디자인시스템 레퍼런스 분석 기준
구현 방향을 잡기 위해 Ant Design, Chakra UI v3, Mantine, Next.js 총 4개 디자인시스템의 Image 컴포넌트를 분석했습니다.
노션 링크 내부에 각 레퍼런스별 공식문서와 github 링크, 파악한 내용 정리한 부분을 첨부합니다.
채택한 방향
팀원들과 논의를 하고 싶은 부분
현재는 visibility: hidden으로 공간을 유지하면서 내용을 숨기는 방식으로 구현했습니다. Ant Design과 동일한 방식입니다. 기본 fallback UI를 내장하는 방향으로 가면 에러 상태가 더 명확해지고 일관성도 높아질 것 같아서, 디자인팀과 UI 형태를 논의한 후 추가하는 방향을 제안합니다.
현재는 사용자가 직접 값을 입력하는 방식으로 열어뒀습니다. 토큰 관련해서 논의 후에 진행하는 것이 좋을 것 같습니다.
현재 서비스에서 비율로 처리하는 케이스가 한 곳으로 CSS로도 대응 가능한 수준이라 제외했습니다. 실제로 쓰다가 불편하다는 피드백이 오면 그때 추가하는 방향을 고민해봤습니다.
현재는
늦게 올려서 죄송합니다. 작업이 처음이라 부족한 부분이 많을 것 같습니다.. 피드백 주시면 최대한 열심히 찾아보겠습니다.