-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: 优化处理自定义渲染内容 #6904
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
fix: 优化处理自定义渲染内容 #6904
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthrough此次变更主要更新了 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/image-viewer/index.zh.md (1)
90-178: 修复重复内容问题文档中存在大量重复内容,从第 90 行开始几乎完全重复了前面的内容。这会影响文档的可读性和维护性。
需要删除重复的内容:
-# ImageViewer 图片查看器 - -通过放大查看图片全貌。 - -## 何时使用 - -需要点开图片查看细节,配合缩略图使用。 - -## 示例 - -<code src="./demos/demo1.tsx"></code> - -## ImageViewer - -| 属性 | 说明 | 类型 | 默认值 | 版本 | -| --- | --- | --- | --- | --- | -| afterClose | 完全关闭后触发 | `() => void` | - | | -| classNames | 语义化 class | `{ mask?:string,body?:string }` | - | 5.33.1 | -| getContainer | 指定挂载的 HTML 节点,默认为 `null` 渲染到当前节点 | `HTMLElement \| () => HTMLElement \| null` | `null` | | -| image | 图片资源的 `url` | `string` | - | | -| maxZoom | 最大缩放比例 | `number \| 'auto'` | `3` | | -| onClose | 关闭时触发 | `() => void` | - | | -| renderFooter | 渲染底部额外内容 | `(image: string) => ReactNode` | - | | -| imageRender | 自定义渲染内容 | `(image: string, { index }: { index: number }) => ReactNode` | - | 5.39.0 | -| partialCustomRender | 是否部分自定义渲染(`true`: 自定义但保留图片拖动,`false`: 完全自定义,不处理拖拽) | `boolean` | `true` | | -| visible | 是否显示 | `boolean` | `false` | | - -## ImageViewer.Multi - -| 属性 | 说明 | 类型 | 默认值 | 版本 | -| --- | --- | --- | --- | --- | -| classNames | 语义化 class | `{ mask?:string,body?:string }` | - | 5.33.1 | -| defaultIndex | 默认显示第几张图片 | `number` | `0` | | -| images | 图片资源的 url 列表 | `string[]` | - | | -| onIndexChange | 切换图片时触发 | `(index: number) => void` | - | | -| renderFooter | 渲染底部额外内容 | `(image: string, index: number) => ReactNode` | - | | -| imageRender | 自定义渲染内容 | `(image: string, { index }: { index: number }) => ReactNode` | - | | -| partialCustomRender | 是否部分自定义渲染(`true`: 自定义但保留图片拖动,`false`: 完全自定义,不处理拖拽) | `boolean` | `true` | | - -其他属性同 `ImageViewer`,但是去掉了 `image` 属性。 - -### Ref - -| 属性 | 说明 | 类型 | -| ------- | -------------- | --------------------------------------------- | -| swipeTo | 切换到指定索引 | `(index: number, immediate: boolean) => void` | - -## 指令式 - -相比于上文中组件式的使用方式,指令式更加方便也更加常用,在大多数情况下,都推荐使用这种方式: - -```ts | pure -const handler = ImageViewer.show(props) -const handlerMulti = ImageViewer.Multi.show(props) -``` - -可以通过调用 `ImageViewer` 上的 `show` 方法直接进入图片查看。其中 `props` 参数的类型同上表,但不支持传入 `visible` 属性。当查看器被关闭后,组件实例会自动销毁。 - -`show` 方法的返回值为一个组件控制器,包含以下属性: - -| 属性 | 说明 | 类型 | -| ----- | -------------- | ------------ | -| close | 关闭图片查看器 | `() => void` | - -如果你想关闭全部通过指令式创建出来的 ImageViewer,可以调用 `ImageViewer.clear()`。 - -## FAQ - -### 为什么我更新了 `defaultIndex` 的值,但是 ImageViewer.Multi 显示的图片并没有切换? - -ImageViewer.Multi 是一个[非受控](https://reactjs.org/docs/glossary.html#controlled-vs-uncontrolled-components)的组件,`defaultIndex` 只会在组件加载的时候读取一次,在此之后,如果你修改了 `defaultIndex` 的值,并不会对组件有任何的影响。因此,下面这种写法并不能起到切换图片的效果: - -```jsx -<Button - onClick={() => { - setIndex(i => i + 1) - }} -> - Next -</Button> - -<ImageViewer.Multi - images={images} - defaultIndex={index} -/> -``` - -你可以使用 ref 来对 ImageViewer.Multi 进行手动的操作,也可以考虑使用 `ImageViewer.show()`。
🧹 Nitpick comments (1)
src/components/image-viewer/slide.tsx (1)
306-330:domRender函数实现需要优化函数逻辑正确,但存在一些性能和可维护性问题:
- 性能问题:对复杂的 DOM 树进行递归克隆可能影响性能
- 类型安全:
recursiveClone函数的参数和返回值类型可以更精确- 边界情况处理:需要考虑 children 为数组、字符串或其他类型的情况
建议优化实现:
- const domRender = (dom: React.ReactElement): React.ReactElement => { + const domRender = (dom: React.ReactElement): React.ReactElement => { // 完全放开自定义render,不需要需要将ref应用到img上 if (!partialCustomRender) return dom // 自定义但是保留图片拖动 let refApplied = false - function recursiveClone(element: React.ReactElement): React.ReactElement { + function recursiveClone(element: React.ReactElement): React.ReactElement { if (!React.isValidElement(element)) return element - if (['img', 'video'].includes(element.type) && !refApplied) { + if (['img', 'video'].includes(element.type as string) && !refApplied) { refApplied = true return React.cloneElement(element, { ref: imgRef }) } const children = element?.props?.children if (children) { + // 优化:提前检查是否已经应用了 ref,避免不必要的递归 + if (refApplied) return element + const newChildren = React.Children.map(children, child => React.isValidElement(child) ? recursiveClone(child) : child ) return React.cloneElement(element, undefined, newChildren) } return element } return recursiveClone(dom) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/image-viewer/image-viewer.tsx(5 hunks)src/components/image-viewer/index.en.md(2 hunks)src/components/image-viewer/index.zh.md(2 hunks)src/components/image-viewer/slide.tsx(3 hunks)src/components/image-viewer/slides.tsx(2 hunks)
🧰 Additional context used
🪛 LanguageTool
src/components/image-viewer/index.zh.md
[uncategorized] ~45-~45: A punctuation mark might be missing here.
Context: ...--------------------------------------- | | swipeTo | 切换到指定索引 | `(index: number, ...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
src/components/image-viewer/index.zh.md
90-90: Multiple headings with the same content
(MD024, no-duplicate-heading)
🔇 Additional comments (14)
src/components/image-viewer/slides.tsx (2)
22-22: 类型定义正确新增的
partialCustomRender属性类型定义正确,可选属性符合预期。
106-106: 属性传递实现正确正确地将
partialCustomRender属性传递给子组件Slide,保持了组件间的数据流一致性。src/components/image-viewer/index.en.md (2)
25-25: 文档描述准确
partialCustomRender属性的文档描述清晰准确,正确说明了true和false状态下的不同行为。
40-40: 多图组件文档一致
ImageViewer.Multi组件的partialCustomRender属性文档与单图组件保持一致,描述准确。src/components/image-viewer/image-viewer.tsx (5)
31-31: 类型定义正确
partialCustomRender属性添加到ImageViewerProps接口中,类型声明正确。
42-42: 默认值设置合理将
partialCustomRender默认值设置为true是合理的选择,既保持了向后兼容性,又提供了预期的拖拽功能。中文注释清晰说明了功能含义。
69-69: 属性传递正确正确地将
partialCustomRender属性传递给Slide组件。
95-95: 多图组件类型定义正确
MultiImageViewerProps中正确添加了partialCustomRender属性,类型定义与单图组件保持一致。
151-151: 多图组件属性传递正确正确地将
partialCustomRender属性传递给Slides组件。src/components/image-viewer/slide.tsx (3)
20-20: 类型定义正确
partialCustomRender属性添加到 Props 接口中,类型声明正确。
25-26: 解构赋值清晰正确地从 props 中解构出
partialCustomRender属性,代码结构清晰。
342-342: DOM 渲染逻辑正确正确地在自定义渲染分支中使用
domRender函数处理 DOM 元素,逻辑清晰。src/components/image-viewer/index.zh.md (2)
25-25: 属性文档描述准确
partialCustomRender属性的中文文档描述清晰准确,正确说明了两种模式的区别。
38-38: 多图组件文档一致
ImageViewer.Multi组件的partialCustomRender属性文档描述准确。
|
` if (['img', 'video'].includes(element.type) && !refApplied) { const children = element?.props?.children 我处理了下异常的问题,你们查看下然后合并下代码吧 |
2025-07-02_155200_739.mp4 |
|
给 feature 加个 test 看看 |
没懂你的意思哎 |
|
这个在你自定义的部分直接 |
不行吧,我是放大了在拖动的时候,稍微动下就缩小了,有时候稍微动下就换图片了,这个停止传播,是不让他受影响吗? |
|
而且我只是想自定义渲染,但保留内部的拖动事件,其他不变,如果我自己完全自定义的,需要重写拖动事件,效果没有内部的好用,这个我也试过了,除非我这部分不用,或者重新这个组件,我看了下内部代码,根据拿到contentRef和imgRef去处理这块的,那么我认为,在有ref的时候就使用自身的,没有的就使用内部的 |
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6904 +/- ##
==========================================
+ Coverage 92.79% 92.91% +0.12%
==========================================
Files 336 336
Lines 7244 7326 +82
Branches 1821 1854 +33
==========================================
+ Hits 6722 6807 +85
+ Misses 486 483 -3
Partials 36 36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| const domRender = (dom: React.ReactElement): React.ReactElement => { | ||
| // 完全放开自定义 render,不需要需要将ref应用到img上 | ||
| if (!partialCustomRender) return dom |
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.
看起来不需要多一个属性,既然不要绑定 ref。直接 HOC 一个就行了:
https://codesandbox.io/p/sandbox/clever-keller-q3gk3r?file=%2Fapp.tsx%3A29%2C8
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.
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.
imageRender 方法本身就是交由开发者自定义的。不能说因为我需要它不支持 xxx 就加一个 xxx 属性,那未来有一个需求就多一个 API 会非常混乱。
这个问题的解法就是 HOC 一个组件,所以问“没有HOC呢”那就是用 HOC 即可。
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.
我没说hoc不行,我不是说了吗,如果不加属性的话,就说明下这个地方,如果完全自定义的话需要使用HOC
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.
那我把这个属性去掉?
React.cloneElement(element, { ref:element.ref?element.ref: imgRef }) 我改下,如果有Ref 就给保留,原本是想通过属性直接隔绝调,如果不要的话我就这么写了,没有ref用组件内的,如果有就保留用户自身的ref
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.
imageRender方法本身就是交由开发者自定义的。不能说因为我需要它不支持 xxx 就加一个 xxx 属性,那未来有一个需求就多一个 API 会非常混乱。这个问题的解法就是 HOC 一个组件,所以问“没有HOC呢”那就是用 HOC 即可。
`
const domRender = (dom: React.ReactElement): React.ReactElement => {
let refApplied = false
function recursiveClone(element: React.ReactElement): React.ReactElement {
if (!React.isValidElement(element)) return element
if (
typeof element.type === 'string' &&
['img', 'video'].includes(element.type) &&
!refApplied
) {
refApplied = true
return React.cloneElement(element, { ref: (element as any)?.ref || imgRef } as any)
}
const children = (element.props as { children?: React.ReactNode })
?.children
if (children) {
const newChildren = React.Children.map(children, child =>
React.isValidElement(child) ? recursiveClone(child) : child
)
return React.cloneElement(element, undefined, newChildren)
}
return element
}
return recursiveClone(dom)
}
`
代码改成这样你看下,另外
return React.cloneElement(element, { ref: (element as any)?.ref || imgRef } as any) 这部分的ts,该怎么优化呢
984507092
left a comment
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.
那这样也可以的,起初我想的是,如果我这边新HOC了,但是这边检测到一个新的dom,给遍历了,简单来说我没有HOC直接dom,那不就不行吗
|
按照这个 case 只要加个 FAQ 说一下做法就行了 |
| | onIndexChange | Triggered when the picture is switched | `(index: number) => void` | - | | | ||
| | renderFooter | Render extra content on footer | `(image: string, index: number) => ReactNode` | - | | | ||
| | imageRender | Custom rendering content | `(image: string, { index }: { index: number }) => ReactNode` | - | | | ||
| | imageRender | Custom rendering content | `(image: string, { index }: { index: number }) => ReactNode,` | - | | |
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.
这个 , 多余
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.
这个
,多余
好的
在哪里加? |
这边还有那些问题吗,然后我一起改下,这个功能麻烦审核下,然后合并下呗 |
| > | ||
| {customRendering ? ( | ||
| customRendering | ||
| {customRendering && React.isValidElement(customRendering) ? ( |
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.
不需要检查,原本逻辑是可以接受数组的。这么检查会 break 掉也没有意义。
| {...props} | ||
| onZoomChange={(nextZoom: number) => { | ||
| G.nextZoom = nextZoom | ||
| G.lastZoom = nextZoom // 新增这一行 |
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.
这个直接用 nextZoom 看起来就够了?
| }) | ||
| }) | ||
|
|
||
| describe('自定义渲染和HOC场景', () => { |
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.
参考其他测试用例,用英文描述
| expect(G.lastZoom).toBeGreaterThan(1) | ||
| }) | ||
|
|
||
| test('HOC 封装的 ImageViewer 也能正常工作', async () => { |
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.
这个测试里应该需要测试 zoom 在 Image 上无效才对
| const customRendering = | ||
| typeof imageRender === 'function' && | ||
| imageRender(props.image, { index } as { index: number }) | ||
| imageRender(props.image, { ref: imgRef, index } as { |
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.
外侧定义没加,这里不需要 as,外面对了这里就没问题了。原本的 as 写法不太好。
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.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/image-viewer/image-viewer.tsx(3 hunks)src/components/image-viewer/index.en.md(2 hunks)src/components/image-viewer/index.zh.md(2 hunks)src/components/image-viewer/slide.tsx(2 hunks)src/components/image-viewer/slides.tsx(2 hunks)src/components/image-viewer/tests/image-viewer.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/image-viewer/slide.tsx
- src/components/image-viewer/index.zh.md
- src/components/image-viewer/index.en.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/image-viewer/tests/image-viewer.test.tsx (2)
src/components/image-viewer/image-viewer.tsx (1)
ImageViewer(46-82)src/components/image-viewer/demos/images.tsx (1)
demoImages(1-6)
🔇 Additional comments (3)
src/components/image-viewer/slides.tsx (2)
5-5: 正确导入 RefObject 类型正确添加了
RefObject类型导入,支持新的 ref 参数功能。
22-25: 类型签名更新正确
imageRender函数的类型签名正确更新,新增的ref参数为可选参数,类型为RefObject<HTMLImageElement>,符合 PR 目标 - 允许自定义渲染时保持拖拽行为。src/components/image-viewer/image-viewer.tsx (1)
2-2: 正确导入 RefObject 类型正确添加了
RefObject类型导入以支持新的 ref 参数功能。
| imageRender?: ( | ||
| image: string, | ||
| { ref, index }: { ref?: RefObject<HTMLImageElement>; index: number } | ||
| ) => ReactNode |
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.
💡 Verification agent
🧩 Analysis chain
类型签名不一致需要确认
发现 ImageViewerProps 和 MultiImageViewerProps 中 imageRender 的 ref 参数类型定义不一致:
ImageViewerProps:ref?: RefObject<HTMLImageElement>(可选)MultiImageViewerProps:ref: RefObject<HTMLImageElement>(必需)
这种不一致可能会给开发者造成困惑。请确认这是有意的设计决策还是应该保持一致。
Also applies to: 94-97
🏁 Script executed:
#!/bin/bash
# 检查其他相关文件中 imageRender 的类型定义是否一致
echo "检查 slide.tsx 中的 imageRender 类型定义:"
rg -A 3 "imageRender.*:" src/components/image-viewer/slide.tsx
echo "检查文档中的示例用法:"
rg -A 5 -B 5 "imageRender.*ref" src/components/image-viewer/index.*.mdLength of output: 2652
🏁 Script executed:
#!/bin/bash
# 定位 MultiImageViewerProps 定义的位置
echo "查找 MultiImageViewerProps 定义:"
rg -n "interface MultiImageViewerProps" src/components/image-viewer
# 查看 MultiImageViewerProps 中 imageRender 的类型签名
echo "MultiImageViewerProps 中 imageRender 类型定义:"
rg -A2 "imageRender" src/components/image-viewer/multi-image-viewer.tsxLength of output: 392
🏁 Script executed:
#!/bin/bash
# 列出 image-viewer 目录下所有文件,确认 MultiImageViewer 相关文件名
echo "目录结构:"
ls -R src/components/image-viewer
# 全局搜索 MultiImageViewerProps
echo "搜索 MultiImageViewerProps 定义:"
rg -n "MultiImageViewerProps" .
# 如果找不到,搜索 MultiImageViewer 关键字
echo "搜索组件名 MultiImageViewer:"
rg -n "MultiImageViewer" -g "*.tsx" .Length of output: 2157
🏁 Script executed:
#!/bin/bash
# 查看 MultiImageViewerProps 的定义及 imageRender 签名
echo "---- MultiImageViewerProps 定义 ----"
rg -n "export type MultiImageViewerProps" -A10 src/components/image-viewer/image-viewer.tsx
echo "---- imageRender 在 image-viewer.tsx 中的定义 ----"
rg -n "imageRender" -A5 -B5 src/components/image-viewer/image-viewer.tsxLength of output: 2471
请统一 imageRender 中 ref 的可选性
目前在 src/components/image-viewer/image-viewer.tsx 中:
ImageViewerProps定义:imageRender?: ( image: string, { ref, index }: { ref?: RefObject<HTMLImageElement>; index: number } ) => ReactNode
MultiImageViewerProps定义:imageRender?: ( image: string, { ref, index }: { ref: RefObject<HTMLImageElement>; index: number } ) => ReactNode
- 文档 (
index.en.md/index.zh.md) 均使用了ref?: RefObject<HTMLImageElement>。
请将 MultiImageViewerProps 中的 ref 修改为可选,以与单图渲染、文档保持一致。
需要修改的位置:
- 文件:
src/components/image-viewer/image-viewer.tsx - 约第 96 行
建议改动:
- imageRender?: (
- image: string,
- { ref, index }: { ref: RefObject<HTMLImageElement>; index: number }
- ) => ReactNode
+ imageRender?: (
+ image: string,
+ { ref, index }: { ref?: RefObject<HTMLImageElement>; index: number }
+ ) => ReactNode🤖 Prompt for AI Agents
In src/components/image-viewer/image-viewer.tsx around line 96, the ref
parameter in the imageRender function of MultiImageViewerProps is currently
required but should be optional to match the definition in ImageViewerProps and
the documentation. Modify the type of ref from RefObject<HTMLImageElement> to
ref?: RefObject<HTMLImageElement> to make it optional, ensuring consistency
across the codebase and docs.
| test('rendering with imageRender and ref', () => { | ||
| function App() { | ||
| return ( | ||
| <ImageViewer.Multi | ||
| images={demoViewImages} | ||
| visible | ||
| imageRender={(image, info) => ( | ||
| <div | ||
| className={`customize-preview-node-${info.index}`} | ||
| ref={info.ref} | ||
| /> | ||
| )} | ||
| /> | ||
| ) | ||
| } | ||
| render(<App />) | ||
| expect(document.querySelector('.customize-preview-node-0')).toBeTruthy() | ||
| }) |
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.
🛠️ Refactor suggestion
测试覆盖不够全面
当前测试只验证了自定义元素是否渲染,但没有验证 ref 是否正确传递和生效。建议增强测试以验证 ref 的实际功能:
- 验证
info.ref参数确实被传递 - 验证 ref 被正确绑定到 DOM 元素
- 测试 ref 对拖拽行为的影响
test('rendering with imageRender and ref', () => {
+ const mockRef = React.createRef<HTMLImageElement>()
function App() {
return (
<ImageViewer.Multi
images={demoViewImages}
visible
- imageRender={(image, info) => (
+ imageRender={(image, info) => {
+ // 验证 ref 参数确实被传递
+ expect(info.ref).toBeDefined()
+ return (
<div
className={`customize-preview-node-${info.index}`}
ref={info.ref}
/>
- )}
+ )
+ }}
/>
)
}
render(<App />)
expect(document.querySelector('.customize-preview-node-0')).toBeTruthy()
})Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/image-viewer/tests/image-viewer.test.tsx around lines 249 to
266, the test only checks if the custom element renders but does not verify if
the ref is correctly passed and functional. Enhance the test by asserting that
the info.ref parameter is defined, confirming the ref is properly attached to
the DOM element, and adding a test to check how the ref affects drag behavior,
ensuring the ref works as intended.
| test('rendering with imageRender and ref', () => { | ||
| function App() { | ||
| return ( | ||
| <ImageViewer | ||
| image={demoImages[0]} | ||
| visible | ||
| imageRender={(image, info) => ( | ||
| <div | ||
| className={`customize-preview-node-${info.index}`} | ||
| ref={info.ref} | ||
| /> | ||
| )} | ||
| /> | ||
| ) | ||
| } | ||
| render(<App />) | ||
| expect(document.querySelector('.customize-preview-node-0')).toBeTruthy() | ||
| }) |
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.
🛠️ Refactor suggestion
测试重复且覆盖不完整
此测试与 ImageViewer.Multi 的测试几乎相同,存在重复。更重要的是,都没有验证 ref 的核心功能 - 即保持拖拽行为在自定义渲染时正常工作。
建议将此测试改为验证 ref 对拖拽行为的实际影响:
- test('rendering with imageRender and ref', () => {
+ test('imageRender with ref preserves drag behavior when zoomed', () => {
function App() {
return (
<ImageViewer
image={demoImages[0]}
visible
- imageRender={(image, info) => (
- <div
- className={`customize-preview-node-${info.index}`}
- ref={info.ref}
- />
- )}
+ imageRender={(image, info) => (
+ <img
+ src={image}
+ className={`custom-image-${info.index}`}
+ ref={info.ref}
+ />
+ )}
/>
)
}
render(<App />)
- expect(document.querySelector('.customize-preview-node-0')).toBeTruthy()
+
+ // 放大图片
+ act(() => {
+ triggerPinch([2, 0])
+ })
+ expect(G.nextZoom).toBeGreaterThan(1)
+
+ // 验证拖拽不会导致缩放重置
+ const customImage = document.querySelector('.custom-image-0')
+ expect(customImage).toBeTruthy()
+
+ mockDrag(customImage as HTMLElement, [
+ { clientX: 100 },
+ { clientX: 200 },
+ ])
+
+ // 拖拽后缩放应该保持
+ expect(G.nextZoom).toBeGreaterThan(1)
})📝 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.
| test('rendering with imageRender and ref', () => { | |
| function App() { | |
| return ( | |
| <ImageViewer | |
| image={demoImages[0]} | |
| visible | |
| imageRender={(image, info) => ( | |
| <div | |
| className={`customize-preview-node-${info.index}`} | |
| ref={info.ref} | |
| /> | |
| )} | |
| /> | |
| ) | |
| } | |
| render(<App />) | |
| expect(document.querySelector('.customize-preview-node-0')).toBeTruthy() | |
| }) | |
| test('imageRender with ref preserves drag behavior when zoomed', () => { | |
| function App() { | |
| return ( | |
| <ImageViewer | |
| image={demoImages[0]} | |
| visible | |
| imageRender={(image, info) => ( | |
| <img | |
| src={image} | |
| className={`custom-image-${info.index}`} | |
| ref={info.ref} | |
| /> | |
| )} | |
| /> | |
| ) | |
| } | |
| render(<App />) | |
| // 放大图片 | |
| act(() => { | |
| triggerPinch([2, 0]) | |
| }) | |
| expect(G.nextZoom).toBeGreaterThan(1) | |
| // 验证拖拽不会导致缩放重置 | |
| const customImage = document.querySelector('.custom-image-0') | |
| expect(customImage).toBeTruthy() | |
| mockDrag(customImage as HTMLElement, [ | |
| { clientX: 100 }, | |
| { clientX: 200 }, | |
| ]) | |
| // 拖拽后缩放应该保持 | |
| expect(G.nextZoom).toBeGreaterThan(1) | |
| }) |
🤖 Prompt for AI Agents
In src/components/image-viewer/tests/image-viewer.test.tsx around lines 312 to
329, the test duplicates coverage of ImageViewer.Multi tests and does not verify
the core functionality of the ref, which is to maintain drag behavior during
custom rendering. Modify the test to simulate drag events on the element
referenced by ref and assert that drag behavior works correctly, ensuring the
ref is properly applied and functional in the custom imageRender prop.
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.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/image-viewer/tests/image-viewer.test.tsx (1)
312-329: ImageViewer 测试用例存在同样问题与 Multi 版本类似,建议使用英文描述并增强测试验证。
- test('rendering with imageRender and ref', () => { + test('should render with imageRender and ref parameter', () => {
🧹 Nitpick comments (1)
src/components/image-viewer/tests/image-viewer.test.tsx (1)
249-266: 测试用例添加合理,但建议使用英文描述测试用例正确验证了
imageRender函数接收ref参数的功能。根据之前的评审意见,建议将测试描述改为英文以保持一致性。- test('rendering with imageRender and ref', () => { + test('should render with imageRender and ref parameter', () => {另外,当前测试只验证了元素存在,建议增加对
ref实际绑定的验证:render(<App />) expect(document.querySelector('.customize-preview-node-0')).toBeTruthy() + // Could add verification that ref is actually bound to the element
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/image-viewer/image-viewer.tsx(3 hunks)src/components/image-viewer/index.en.md(2 hunks)src/components/image-viewer/index.zh.md(2 hunks)src/components/image-viewer/slide.tsx(2 hunks)src/components/image-viewer/slides.tsx(2 hunks)src/components/image-viewer/tests/image-viewer.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/image-viewer/slide.tsx
- src/components/image-viewer/index.en.md
- src/components/image-viewer/index.zh.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/image-viewer/tests/image-viewer.test.tsx (2)
src/components/image-viewer/image-viewer.tsx (1)
ImageViewer(46-82)src/components/image-viewer/demos/images.tsx (1)
demoImages(1-6)
🔇 Additional comments (2)
src/components/image-viewer/slides.tsx (1)
5-5: 类型定义更新合理添加
RefObject导入和更新imageRender函数签名以包含可选的ref参数是正确的。这个改动支持自定义渲染时保留原生拖拽功能,类型定义准确且保持了向后兼容性。Also applies to: 22-25
src/components/image-viewer/image-viewer.tsx (1)
2-2: 导入更新正确添加
RefObject导入以支持新的类型定义是正确的。
| imageRender?: (image: string, { index }: { index: number }) => ReactNode | ||
| imageRender?: ( | ||
| image: string, | ||
| { ref, index }: { ref?: RefObject<HTMLImageElement>; index: number } |
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.
ref 应该始终是有的,不需要 optional
| imageRender={(image, info) => ( | ||
| <div | ||
| className={`customize-preview-node-${info.index}`} | ||
| ref={info.ref} |
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.
这里应该expect ref 是一个 object,否则有没有 ref 测试都能过
| | onIndexChange | Triggered when the picture is switched | `(index: number) => void` | - | | | ||
| | renderFooter | Render extra content on footer | `(image: string, index: number) => ReactNode` | - | | | ||
| | imageRender | Custom rendering content | `(image: string, { index }: { index: number }) => ReactNode` | - | | | ||
| | imageRender | Custom rendering content | `(image: string, { ref,index }: {ref?:RefObject<HTMLImageElement>, index: number }) => ReactNode` | - | | |
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.
文档这边?忘记改了……
| imageRender?: (image: string, { index }: { index: number }) => ReactNode | ||
| imageRender?: ( | ||
| image: string, | ||
| { ref, index }: { ref: RefObject<HTMLImageElement>; index: number } |
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.
格式化一下
* fix: 优化处理自定义渲染内容 * fix: 优化代码ts类型 * fix: 去掉新增属性限制完全自定义,知道用户使用HOC * fix: 移除多余逗号 * fix: 修改md和格式化 * fix: 增加FAQ备注 * fix: 优化ref逻辑处理问题 * fix: 优化ref逻辑处理问题 * fix: 增加test case * fix: 增加test case * fix: 增加test case * fix: 优化注释和代码 * fix: 修改ref逻辑,让用户自行决定是否使用 * fix: 修改ref逻辑,让用户自行决定是否使用 * fix: 调整ts类型 * fix: 格式化内容


问题:
在自定渲染内容的时候,放大的时候,点击或者移动会缩小,或者切换
新增和优化
修改了下,图片预览在自定义的时候,保留图片拖动问题,
新增了是否完全使用自定义或使用自定义但保留拖动问题
` const domRender = (dom: React.ReactElement): React.ReactElement => {
// 完全放开自定义render,不需要需要将ref应用到img上
if (!partialCustomRender) return dom
}`
Summary by CodeRabbit
文档
ImageViewer.Multi组件的imageRender属性签名说明,新增ref参数,并在中英文文档中补充了相关 FAQ,解释自定义预览如何支持手势操作,提供了示例代码。样式
重构
imageRender回调参数,支持传递ref,便于自定义渲染时绑定引用,实现手势等默认行为。测试
ref参数的imageRender自定义渲染函数,验证组件渲染和引用绑定。杂项