-
Notifications
You must be signed in to change notification settings - Fork 267
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
feat(ImagePreview): 支持taro下缩放效果。 #2939
base: feat_v3.x
Are you sure you want to change the base?
Conversation
步骤概述演练此次更改主要涉及图片预览组件( 变更
可能相关的 PR
建议审阅者
诗歌
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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
🧹 Nitpick comments (2)
src/packages/imagepreview/imagepreview.tsx (2)
121-141
: 建议将魔法数字提取为常量为了提高代码的可维护性,建议将以下值提取为命名常量:
- 双击时间阈值 (300ms)
- 缩放倍数 (2x, 3x)
- 缩放阈值 (1.1)
+const DOUBLE_TAP_THRESHOLD = 300; // ms +const DEFAULT_SCALE = 1; +const DOUBLE_TAP_SCALE = 2; +const MAX_SCALE = 3; +const MIN_SCALE_THRESHOLD = 1.1; const onTouchStart = (event: TouchEvent) => { const { touches } = event const events = touches[0] const events2 = touches[1] const curTouchTime = Date.now() - if (curTouchTime - lastTouchEndTime < 300) { + if (curTouchTime - lastTouchEndTime < DOUBLE_TAP_THRESHOLD) { const store1 = store - store1.scale = store1.scale === 1 ? 2 : 1 + store1.scale = store1.scale === DEFAULT_SCALE ? DOUBLE_TAP_SCALE : DEFAULT_SCALE scaleNow() }Also applies to: 145-164
266-287
: 建议改进类型安全性当前实现中的类型断言可以通过使用类型守卫(Type Guard)来改进:
+type MediaItem = { + type: 'video' | 'image'; + data: VideoOption | ImageOption; +}; +const isVideoOption = (item: MediaItem): item is { type: 'video', data: VideoOption } => { + return item.type === 'video'; +}; {[ ...videos.map((item) => ({ type: 'video', data: item })), ...images.map((item) => ({ type: 'image', data: item })), ] .sort((a, b) => (a.data.index ?? 0) - (b.data.index ?? 0)) .map((item, index) => ( <SwiperItem key={index}> - {item.type === 'video' ? ( + {isVideoOption(item) ? ( <Video - source={(item.data as VideoOption).source} - options={(item.data as VideoOption).options} + source={item.data.source} + options={item.data.options} onClick={closeOnImg} /> ) : ( <Image - src={(item.data as ImageOption).src} + src={item.data.src} draggable={false} onClick={closeOnImg} /> )} </SwiperItem> ))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/packages/imagepreview/imagepreview.scss
(1 hunks)src/packages/imagepreview/imagepreview.taro.tsx
(6 hunks)src/packages/imagepreview/imagepreview.tsx
(8 hunks)src/packages/notify/Notification.tsx
(1 hunks)src/packages/toast/Notification.tsx
(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/packages/notify/Notification.tsx
- src/packages/toast/Notification.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/packages/imagepreview/imagepreview.taro.tsx
[error] 238-240: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 245-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (3)
src/packages/imagepreview/imagepreview.tsx (1)
46-47
: 类型定义优化得当!通过添加 index 属性来支持自定义排序是一个很好的改进。
src/packages/imagepreview/imagepreview.taro.tsx (1)
290-292
: 平台特定功能处理得当!针对非 Web 环境添加 showMenuByLongpress 支持是很好的做法。
src/packages/imagepreview/imagepreview.scss (1)
65-67
: 样式约束设置合理!添加 max-width 约束确保了组件在各种屏幕尺寸下的正确显示。
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat_v3.x #2939 +/- ##
=============================================
+ Coverage 85.58% 86.07% +0.48%
=============================================
Files 277 277
Lines 18133 18088 -45
Branches 2754 2766 +12
=============================================
+ Hits 15519 15569 +50
+ Misses 2609 2514 -95
Partials 5 5 ☔ View full report in Codecov by Sentry. |
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
🧹 Nitpick comments (1)
src/packages/imagepreview/__test__/imagepreview.spec.tsx (1)
49-62
: 很好的测试设置函数实现!通过引入
setup
函数统一管理组件渲染,减少了代码重复,提高了测试代码的可维护性。建议考虑以下优化:
const setup = (props = {}) => { - render( + return render( <ImagePreview images={images} videos={videos} visible closeIcon defaultValue={0} onChange={mockOnChange} onClose={mockOnClose} {...props} /> ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/packages/imagepreview/__test__/imagepreview.spec.tsx
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (3)
src/packages/imagepreview/__test__/imagepreview.spec.tsx (3)
165-194
: 👍 移除了不可靠的计时测试代码移除使用
sleep
函数的测试代码是个好的改进。新的autoPlay
测试使用了更可靠的事件触发方式。
7-162
: 建议补充以下测试场景当前测试用例集合还缺少一些重要场景的覆盖:
- 错误处理测试
- 图片加载失败的情况
- 无效的图片地址
- 可访问性测试
- 键盘导航
- 屏幕阅读器支持
- 边界条件测试
- 空图片数组
- 单张图片时的行为
需要验证这些场景在实际使用中的重要性,是否需要优先添加相关测试用例?
125-150
: 🛠️ Refactor suggestion缺少Taro环境特定的缩放测试
根据PR目标,这个功能是为Taro框架添加的,但当前测试用例可能无法完全覆盖Taro环境的特定行为。建议:
- 添加Taro环境特定的测试用例
- 补充更多缩放场景的测试,如:
- 最大/最小缩放限制
- 双击缩放
- 缩放重置
另外,当前的transform断言不够严谨:
-expect((swiperIndicator as HTMLElement).style.transform).toContain('scale(') +expect((swiperIndicator as HTMLElement).style.transform).toMatch(/scale\([\d.]+\)/)
fireEvent.click(closeIcon as Element) | ||
// await waitFor(() => expect(mockOnClose).toHaveBeenCalledOnce()) |
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.
需要补充异步断言验证
mockOnClose
的调用验证被注释掉了,建议添加适当的异步断言:
fireEvent.click(closeIcon as Element)
-// await waitFor(() => expect(mockOnClose).toHaveBeenCalledOnce())
+expect(mockOnClose).toHaveBeenCalledTimes(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.
fireEvent.click(closeIcon as Element) | |
// await waitFor(() => expect(mockOnClose).toHaveBeenCalledOnce()) | |
fireEvent.click(closeIcon as Element) | |
expect(mockOnClose).toHaveBeenCalledTimes(1) |
在 H5 中快速滑动图片会触发放大功能 |
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
🧹 Nitpick comments (2)
src/packages/imagepreview/__test__/imagepreview.spec.tsx (2)
8-44
: 建议将测试数据移至单独的文件当前的测试数据(images和videos数组)直接定义在测试文件中,建议将其移至单独的测试数据文件中以提高可维护性和重用性。
+// src/packages/imagepreview/__test__/__fixtures__/testData.ts +export const testImages = [ + { + src: '//m.360buyimg.com/mobilecms/s750x366_jfs/t1/18629/34/3378/144318/5c263f64Ef0e2bff0/0d650e0aa2e852ee.jpg', + }, + // ... +] + +export const testVideos = [ + { + source: { + src: 'https://storage.jd.com/about/big-final.mp4?Expires=3730193075&AccessKey=3LoYX1dQWa6ZXzQl&Signature=ViMFjz%2BOkBxS%2FY1rjtUVqbopbJI%3D', + type: 'video/mp4', + }, + options: { + muted: true, + controls: true, + }, + }, + // ... +]
49-62
: 优化setup函数的类型定义setup函数缺少类型定义,建议添加TypeScript类型以提高代码的可维护性。
-const setup = (props = {}) => { +interface SetupProps extends Partial<ImagePreviewProps> { + // 添加测试特定的属性 +} + +const setup = (props: SetupProps = {}) => { render( <ImagePreview images={images} videos={videos} visible closeIcon defaultValue={0} onChange={mockOnChange} onClose={mockOnClose} {...props} /> ) }
test('renders correctly when visible', async () => { | ||
const { container } = render( | ||
<ImagePreview images={images} visible autoPlay={1000} /> | ||
<ImagePreview | ||
images={images} | ||
videos={videos} | ||
visible | ||
defaultValue={0} | ||
onChange={mockOnChange} | ||
onClose={mockOnClose} | ||
/> | ||
) | ||
expect(screen.getByText('1/6')).toBeInTheDocument() // Assuming pagination is shown | ||
expect((await container).getElementsByTagName('img')[0]).toHaveAttribute( | ||
'src', | ||
'//m.360buyimg.com/mobilecms/s750x366_jfs/t1/18629/34/3378/144318/5c263f64Ef0e2bff0/0d650e0aa2e852ee.jpg' | ||
) | ||
_container = container | ||
}) |
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
完善基础渲染测试的断言
当前的渲染测试只验证了分页文本和第一张图片,建议添加更多断言以确保组件正确渲染所有关键元素。
test('renders correctly when visible', async () => {
const { container } = render(
<ImagePreview
images={images}
videos={videos}
visible
defaultValue={0}
onChange={mockOnChange}
onClose={mockOnClose}
/>
)
expect(screen.getByText('1/6')).toBeInTheDocument()
expect((await container).getElementsByTagName('img')[0]).toHaveAttribute(
'src',
'//m.360buyimg.com/mobilecms/s750x366_jfs/t1/18629/34/3378/144318/5c263f64Ef0e2bff0/0d650e0aa2e852ee.jpg'
)
+ // 验证视频元素
+ const videoElements = container.getElementsByTagName('video')
+ expect(videoElements.length).toBe(2)
+
+ // 验证轮播容器
+ expect(container.querySelector('.nut-swiper')).toBeInTheDocument()
+
+ // 验证图片预览容器
+ expect(container.querySelector('.nut-imagepreview')).toHaveClass('nut-imagepreview-show')
})
📝 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('renders correctly when visible', async () => { | |
const { container } = render( | |
<ImagePreview images={images} visible autoPlay={1000} /> | |
<ImagePreview | |
images={images} | |
videos={videos} | |
visible | |
defaultValue={0} | |
onChange={mockOnChange} | |
onClose={mockOnClose} | |
/> | |
) | |
expect(screen.getByText('1/6')).toBeInTheDocument() // Assuming pagination is shown | |
expect((await container).getElementsByTagName('img')[0]).toHaveAttribute( | |
'src', | |
'//m.360buyimg.com/mobilecms/s750x366_jfs/t1/18629/34/3378/144318/5c263f64Ef0e2bff0/0d650e0aa2e852ee.jpg' | |
) | |
_container = container | |
}) | |
test('renders correctly when visible', async () => { | |
const { container } = render( | |
<ImagePreview | |
images={images} | |
videos={videos} | |
visible | |
defaultValue={0} | |
onChange={mockOnChange} | |
onClose={mockOnClose} | |
/> | |
) | |
expect(screen.getByText('1/6')).toBeInTheDocument() // Assuming pagination is shown | |
expect((await container).getElementsByTagName('img')[0]).toHaveAttribute( | |
'src', | |
'//m.360buyimg.com/mobilecms/s750x366_jfs/t1/18629/34/3378/144318/5c263f64Ef0e2bff0/0d650e0aa2e852ee.jpg' | |
) | |
// 验证视频元素 | |
const videoElements = container.getElementsByTagName('video') | |
expect(videoElements.length).toBe(2) | |
// 验证轮播容器 | |
expect(container.querySelector('.nut-swiper')).toBeInTheDocument() | |
// 验证图片预览容器 | |
expect(container.querySelector('.nut-imagepreview')).toHaveClass('nut-imagepreview-show') | |
}) |
test('autoPlay', async () => { | ||
const { container } = render( | ||
<ImagePreview images={images} videos={videos} visible autoPlay={2000} /> | ||
) | ||
const swiper = container.querySelectorAll('.nut-swiper')[0] | ||
const swiperItem = container.querySelector('.nut-swiper-slide') | ||
triggerDrag(swiper, 220, 0) | ||
expect(swiperItem).toHaveStyle({ | ||
transform: 'translate3d(100%,0,0)', | ||
}) | ||
}) |
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
完善自动播放测试
自动播放测试需要验证定时器和切换效果,当前实现可能不够稳定。建议使用 jest.useFakeTimers()
来控制时间。
test('autoPlay', async () => {
+ jest.useFakeTimers()
const { container } = render(
<ImagePreview images={images} videos={videos} visible autoPlay={2000} />
)
const swiper = container.querySelectorAll('.nut-swiper')[0]
const swiperItem = container.querySelector('.nut-swiper-slide')
- triggerDrag(swiper, 220, 0)
- expect(swiperItem).toHaveStyle({
- transform: 'translate3d(100%,0,0)',
- })
+
+ // 验证初始状态
+ expect(screen.getByText('1/6')).toBeInTheDocument()
+
+ // 等待自动播放
+ jest.advanceTimersByTime(2000)
+
+ // 验证是否切换到下一张
+ expect(screen.getByText('2/6')).toBeInTheDocument()
+
+ jest.useRealTimers()
})
📝 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('autoPlay', async () => { | |
const { container } = render( | |
<ImagePreview images={images} videos={videos} visible autoPlay={2000} /> | |
) | |
const swiper = container.querySelectorAll('.nut-swiper')[0] | |
const swiperItem = container.querySelector('.nut-swiper-slide') | |
triggerDrag(swiper, 220, 0) | |
expect(swiperItem).toHaveStyle({ | |
transform: 'translate3d(100%,0,0)', | |
}) | |
}) | |
test('autoPlay', async () => { | |
jest.useFakeTimers() | |
const { container } = render( | |
<ImagePreview images={images} videos={videos} visible autoPlay={2000} /> | |
) | |
const swiper = container.querySelectorAll('.nut-swiper')[0] | |
const swiperItem = container.querySelector('.nut-swiper-slide') | |
// 验证初始状态 | |
expect(screen.getByText('1/6')).toBeInTheDocument() | |
// 等待自动播放 | |
jest.advanceTimersByTime(2000) | |
// 验证是否切换到下一张 | |
expect(screen.getByText('2/6')).toBeInTheDocument() | |
jest.useRealTimers() | |
}) |
test('handles zooming in and out on touch events', () => { | ||
const { container } = render(<ImagePreview images={images} visible />) | ||
const swiperIndicator = container.querySelector( | ||
'.nut-imagepreview' | ||
) as Element | ||
|
||
// Simulate touch start for zoom in | ||
fireEvent.touchStart(swiperIndicator, { | ||
touches: [ | ||
{ pageX: 100, pageY: 100 }, | ||
{ pageX: 200, pageY: 200 }, | ||
], | ||
}) | ||
|
||
// Simulate touch move for zooming | ||
fireEvent.touchMove(swiperIndicator, { | ||
touches: [ | ||
{ pageX: 100, pageY: 100 }, | ||
{ pageX: 300, pageY: 300 }, | ||
], | ||
}) | ||
|
||
// Verify that scale function has been called or scale state has changed | ||
// Since we don't expose the scale, we may need to check the style if set | ||
expect((swiperIndicator as HTMLElement).style.transform).toContain('scale(') | ||
}) |
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
增强缩放测试用例
当前的缩放测试用例缺少完整的触摸事件流程和具体的缩放比例验证。建议添加更多场景测试和精确的断言。
test('handles zooming in and out on touch events', () => {
const { container } = render(<ImagePreview images={images} visible />)
const swiperIndicator = container.querySelector(
'.nut-imagepreview'
) as Element
+ // 测试放大
fireEvent.touchStart(swiperIndicator, {
touches: [
{ pageX: 100, pageY: 100 },
{ pageX: 200, pageY: 200 },
],
})
fireEvent.touchMove(swiperIndicator, {
touches: [
{ pageX: 100, pageY: 100 },
{ pageX: 300, pageY: 300 },
],
})
- expect((swiperIndicator as HTMLElement).style.transform).toContain('scale(')
+ const transformAfterZoomIn = (swiperIndicator as HTMLElement).style.transform
+ expect(transformAfterZoomIn).toMatch(/scale\([\d.]+\)/)
+
+ // 测试缩小
+ fireEvent.touchStart(swiperIndicator, {
+ touches: [
+ { pageX: 300, pageY: 300 },
+ { pageX: 400, pageY: 400 },
+ ],
+ })
+
+ fireEvent.touchMove(swiperIndicator, {
+ touches: [
+ { pageX: 300, pageY: 300 },
+ { pageX: 350, pageY: 350 },
+ ],
+ })
+
+ const transformAfterZoomOut = (swiperIndicator as HTMLElement).style.transform
+ expect(transformAfterZoomOut).toMatch(/scale\(1\)/)
+
+ // 测试触摸结束
+ fireEvent.touchEnd(swiperIndicator)
})
📝 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('handles zooming in and out on touch events', () => { | |
const { container } = render(<ImagePreview images={images} visible />) | |
const swiperIndicator = container.querySelector( | |
'.nut-imagepreview' | |
) as Element | |
// Simulate touch start for zoom in | |
fireEvent.touchStart(swiperIndicator, { | |
touches: [ | |
{ pageX: 100, pageY: 100 }, | |
{ pageX: 200, pageY: 200 }, | |
], | |
}) | |
// Simulate touch move for zooming | |
fireEvent.touchMove(swiperIndicator, { | |
touches: [ | |
{ pageX: 100, pageY: 100 }, | |
{ pageX: 300, pageY: 300 }, | |
], | |
}) | |
// Verify that scale function has been called or scale state has changed | |
// Since we don't expose the scale, we may need to check the style if set | |
expect((swiperIndicator as HTMLElement).style.transform).toContain('scale(') | |
}) | |
test('handles zooming in and out on touch events', () => { | |
const { container } = render(<ImagePreview images={images} visible />) | |
const swiperIndicator = container.querySelector( | |
'.nut-imagepreview' | |
) as Element | |
// 测试放大 | |
fireEvent.touchStart(swiperIndicator, { | |
touches: [ | |
{ pageX: 100, pageY: 100 }, | |
{ pageX: 200, pageY: 200 }, | |
], | |
}) | |
fireEvent.touchMove(swiperIndicator, { | |
touches: [ | |
{ pageX: 100, pageY: 100 }, | |
{ pageX: 300, pageY: 300 }, | |
], | |
}) | |
const transformAfterZoomIn = (swiperIndicator as HTMLElement).style.transform | |
expect(transformAfterZoomIn).toMatch(/scale\([\d.]+\)/) | |
// 测试缩小 | |
fireEvent.touchStart(swiperIndicator, { | |
touches: [ | |
{ pageX: 300, pageY: 300 }, | |
{ pageX: 400, pageY: 400 }, | |
], | |
}) | |
fireEvent.touchMove(swiperIndicator, { | |
touches: [ | |
{ pageX: 300, pageY: 300 }, | |
{ pageX: 350, pageY: 350 }, | |
], | |
}) | |
const transformAfterZoomOut = (swiperIndicator as HTMLElement).style.transform | |
expect(transformAfterZoomOut).toMatch(/scale\(1\)/) | |
// 测试触摸结束 | |
fireEvent.touchEnd(swiperIndicator) | |
}) |
🤔 这个变动的性质是?
🔗 相关 Issue
💡 需求背景和解决方案
☑️ 请求合并前的自查清单
Summary by CodeRabbit
样式调整
组件优化
性能提升
Date.now()
替代new Date().getTime()
测试增强