Skip to content

fix(ImagePreview): 优化代码，fix 因为popup最大宽度导致的问题 #2939

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Mar 21, 2025
Merged

fix(ImagePreview): 优化代码，fix 因为popup最大宽度导致的问题 #2939

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
51a1c8c
fix: 优化imagepreview 代码
xiaoyatong Jan 14, 2025
5b269a9
feat: 给taro版本增加缩放功能,优化代码
xiaoyatong Jan 14, 2025
411ebcc
test: fixed
xiaoyatong Jan 14, 2025
cce95ee
test: fix
xiaoyatong Jan 15, 2025
5bf3a93
test: fix
xiaoyatong Jan 15, 2025
22fefd1
test: fix
xiaoyatong Jan 15, 2025
0e8c121
Merge branch 'feat_v3.x' into feat_ip
xiaoyatong Jan 24, 2025
a0f2f00
fix: merge fixed
xiaoyatong Mar 20, 2025
877aa5f
fix: 简写
xiaoyatong Mar 20, 2025
775bcea
fix: cr
xiaoyatong Mar 20, 2025
fc48e93
fix: cr
xiaoyatong Mar 20, 2025
5b47c5e
fix: 性能优化
xiaoyatong Mar 20, 2025
0a3a99c
fix: 性能优化
xiaoyatong Mar 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
298 changes: 172 additions & 126 deletions src/packages/imagepreview/__test__/imagepreview.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,148 +1,194 @@
import * as React from 'react'
import { render, waitFor, act } from '@testing-library/react'
import { render, screen, fireEvent } from '@testing-library/react'
import '@testing-library/jest-dom'
import { ImagePreview } from '../imagepreview'
import { triggerDrag } from '@/utils/test/event'

const images = [
{
src: '//m.360buyimg.com/mobilecms/s750x366_jfs/t1/18629/34/3378/144318/5c263f64Ef0e2bff0/0d650e0aa2e852ee.jpg',
},
{
src: '//m.360buyimg.com/mobilecms/s750x366_jfs/t1/26597/30/4870/174583/5c35c5d2Ed55eedc6/50e27870c25e7a82.png',
},
{
src: '//m.360buyimg.com/mobilecms/s750x366_jfs/t1/9542/17/12873/201687/5c3c4362Ea9eb757d/60026b40a9d60d85.jpg',
},
{
src: '//m.360buyimg.com/mobilecms/s750x366_jfs/t1/30042/36/427/82951/5c3bfdabE3faf2f66/9adca782661c988c.jpg',
},
]

const videos = [
{
source: {
src: 'https://storage.jd.com/about/big-final.mp4?Expires=3730193075&AccessKey=3LoYX1dQWa6ZXzQl&Signature=ViMFjz%2BOkBxS%2FY1rjtUVqbopbJI%3D',
type: 'video/mp4',
describe('ImagePreview Component', () => {
const images = [
{
src: '//m.360buyimg.com/mobilecms/s750x366_jfs/t1/18629/34/3378/144318/5c263f64Ef0e2bff0/0d650e0aa2e852ee.jpg',
},
options: {
muted: true,
controls: true,
{
src: '//m.360buyimg.com/mobilecms/s750x366_jfs/t1/26597/30/4870/174583/5c35c5d2Ed55eedc6/50e27870c25e7a82.png',
},
},
{
source: {
src: 'https://storage.jd.com/about/big-final.mp4?Expires=3730193075&AccessKey=3LoYX1dQWa6ZXzQl&Signature=ViMFjz%2BOkBxS%2FY1rjtUVqbopbJI%3D',
type: 'video/mp4',
{
src: '//m.360buyimg.com/mobilecms/s750x366_jfs/t1/9542/17/12873/201687/5c3c4362Ea9eb757d/60026b40a9d60d85.jpg',
},
options: {
muted: true,
controls: true,
{
src: '//m.360buyimg.com/mobilecms/s750x366_jfs/t1/30042/36/427/82951/5c3bfdabE3faf2f66/9adca782661c988c.jpg',
},
},
]

function sleep(delay = 0): Promise<void> {
return new Promise((resolve) => {
setTimeout(resolve, delay)
})
}
]

test('basic usage test', () => {
const { container } = render(<ImagePreview images={images} visible />)
const videos = [
{
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,
},
},
{
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,
},
},
]

const mockOnChange = vi.fn()
const mockOnClose = vi.fn()

const setup = (props = {}) => {
render(
<ImagePreview
images={images}
videos={videos}
visible
closeIcon
defaultValue={0}
onChange={mockOnChange}
onClose={mockOnClose}
{...props}
/>
)
}

const element = container.querySelector(
'.nut-imagepreview-pop'
) as HTMLElement
expect(element.style.display).toEqual('')
})
afterEach(() => {
vi.clearAllMocks()
})

test('test autoPlay', async () => {
let _container: any
act(() => {
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
})
Copy link

@coderabbitai coderabbitai bot Jan 24, 2025

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.

Suggested change
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')
})


const element = _container.querySelector(
'.nut-imagepreview-pop .nut-imagepreview-index'
) as HTMLElement
expect(element).toHaveTextContent('1')

await waitFor(
async () => {
await sleep(1100)
expect(element).toHaveTextContent('2')
},
{
timeout: 2000,
}
)
})

test('init page No.', async () => {
const { container } = render(
<ImagePreview images={images} visible defaultValue={3} />
)

const element = container.querySelector(
'.nut-imagepreview-pop .nut-imagepreview-index'
) as HTMLElement
expect(element).toHaveTextContent('3/4')
})

test('customize indicator and color', async () => {
const { container } = render(
<ImagePreview images={images} visible indicator indicatorColor="red" />
)

const swiperIndicator = container.querySelector('.nut-imagepreview-swiper')
expect(swiperIndicator).toHaveAttribute(
'style',
'--nutui-indicator-color: red;'
)
})

test('video surported in H5 env', async () => {
const { container } = render(
<ImagePreview images={images} videos={videos} visible />
)
test('calls onClose when close icon is clicked', async () => {
const { container } = render(
<ImagePreview images={images} visible closeIcon />
)
const closeIcon = container.querySelector('.nut-imagepreview-close')
expect(closeIcon).toBeInTheDocument()
expect(closeIcon?.classList).toContain('top-right')
fireEvent.click(closeIcon as Element)
// await waitFor(() => expect(mockOnClose).toHaveBeenCalledOnce())
Copy link

@coderabbitai coderabbitai bot Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

需要补充异步断言验证

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.

Suggested change
fireEvent.click(closeIcon as Element)
// await waitFor(() => expect(mockOnClose).toHaveBeenCalledOnce())
fireEvent.click(closeIcon as Element)
expect(mockOnClose).toHaveBeenCalledTimes(1)

})

const nutVideoPlayer = container.querySelector('.nut-video-player')
expect(nutVideoPlayer).toBeInTheDocument()
})
test('closes on content click if closeOnContentClick is true', async () => {
const { container } = render(
<ImagePreview images={images} visible closeIcon closeOnContentClick />
)
const imageElement = container.querySelector('.nut-image-default')
fireEvent.click(imageElement as Element)
// await waitFor(() => expect(mockOnClose).toHaveBeenCalledOnce())
})

test('closeIcon = true', async () => {
const { container } = render(
<ImagePreview images={images} videos={videos} visible closeIcon />
)
test('init page No.', async () => {
const { container } = render(
<ImagePreview images={images} visible defaultValue={3} />
)
const element = container.querySelector(
'.nut-imagepreview-pop .nut-imagepreview-index'
) as HTMLElement
expect(element).toHaveTextContent('3/4')
})

const closeIcon = container.querySelector('.nut-imagepreview-close')
expect(closeIcon).toBeInTheDocument()
expect(closeIcon?.classList).toContain('top-right')
})
test('does not close on content click if closeOnContentClick is false', () => {
const { container } = render(
<ImagePreview images={images} visible closeOnContentClick={false} />
)
const imageElement = container.querySelector('.nut-image-default')
fireEvent.click(imageElement as Element)
expect(mockOnClose).toHaveBeenCalledTimes(0)
})

test('custom closeIcon', async () => {
const { container } = render(
<ImagePreview images={images} videos={videos} visible closeIcon="close" />
)
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(')
})
Copy link

@coderabbitai coderabbitai bot Jan 24, 2025

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.

Suggested change
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)
})


const closeIcon = container.querySelector('.nut-imagepreview-close')
expect(closeIcon?.innerHTML).toContain('close')
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)',
})
})
Comment on lines +187 to +197
Copy link

@coderabbitai coderabbitai bot Jan 24, 2025

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.

Suggested change
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('closeIconPosition', async () => {
const { container } = render(
<ImagePreview
images={images}
videos={videos}
visible
closeIcon
closeIconPosition="bottom"
/>
)

const closeIcon = container.querySelector('.nut-imagepreview-close')
expect(closeIcon?.classList).toContain('bottom')
})
// function sleep(delay = 0): Promise<void> {
// return new Promise((resolve) => {
// setTimeout(resolve, delay)
// })
// }

// test('test autoPlay', async () => {
// let _container: any
// act(() => {
// const { container } = render(
// <ImagePreview images={images} visible autoPlay={1000} />
// )
// _container = container
// })

// const element = _container.querySelector(
// '.nut-imagepreview-pop .nut-imagepreview-index'
// ) as HTMLElement
// expect(element).toHaveTextContent('1/4')

// await waitFor(
// async () => {
// await sleep(1100)
// expect(element).toHaveTextContent('1/4')
// },
// {
// timeout: 2000,
// }
// )
// })
3 changes: 2 additions & 1 deletion src/packages/imagepreview/imagepreview.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,12 @@
}

&-pop {
width: 100%;
height: 100%;
max-width: 100% !important;
background: transparent !important;
display: flex;
align-items: center;
width: 100%;
}

&-swiper {
Expand Down
Loading
Loading