Skip to content
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(Video): web h5 下支持 ref 调用 #2852

Merged
merged 6 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
8 changes: 8 additions & 0 deletions src/packages/empty/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { EmptyAction } from './types'

export const getButtonType = (actions: Array<EmptyAction>, index: number) => {
const action = actions[index]
if (!actions || actions.length === 0) return 'default'
if (action.type) return action.type
actions.length > 1 && index === 0 ? 'default' : 'primary'
}
48 changes: 47 additions & 1 deletion src/packages/video/__tests__/video.spec.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import * as React from 'react'
import { render } from '@testing-library/react'
import { render, fireEvent } from '@testing-library/react'
import '@testing-library/jest-dom'

import { Video } from '../video'
import Button from '@/packages/button'

test('video base info', () => {
const App = () => {
Expand All @@ -26,3 +27,48 @@ test('video base info', () => {
expect(sourceDom?.getAttribute('src')).toBe('xxx.mp4')
expect(container).toMatchSnapshot()
})

test('video ref call', () => {
const pause = vi.fn()
// 确保在每个测试后清理 mock
afterEach(() => {
pause.mockClear()
})
const App = () => {
const source = {
src: 'xxx.mp4',
type: 'video/mp4',
}
const options = {
controls: true,
autoplay: true,
playsinline: true,
loop: true,
poster:
'https://img12.360buyimg.com/ling/s345x208_jfs/t1/168105/33/8417/54825/603df06dEfcddc4cb/21f9f5d0a1b3dad4.jpg.webp',
}
const itemRef = React.useRef<HTMLVideoElement>(null)

return (
<>
<Video
ref={itemRef}
source={source}
options={options}
onPause={pause}
/>
<Button
size="small"
data-testid="emit-click"
onClick={() => itemRef?.current?.pause()}
>
暂停一下
</Button>
</>
)
}

const { getByTestId } = render(<App />)
fireEvent.click(getByTestId('emit-click'))
expect(pause).toHaveBeenCalledTimes(1)
})
6 changes: 3 additions & 3 deletions src/packages/video/demo.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const VideoDemo = () => {
const [translated] = useTranslate({
'zh-CN': {
basic: '基础用法',
autoPlay: '自动播放',
autoPlay: '自动播放,支持 Ref 暂停 和继续播放',
muted: '初始化静音',
cover: '视频封面海报设置',
inline: '行内播放',
Expand All @@ -22,7 +22,7 @@ const VideoDemo = () => {
},
'zh-TW': {
basic: '基礎用法',
autoPlay: '自動播放',
autoPlay: '自動播放,支持 Ref 暫停 和繼續播放',
muted: '初始化靜音',
cover: '視頻封面海報設置',
inline: '行內播放',
Expand All @@ -31,7 +31,7 @@ const VideoDemo = () => {
},
'en-US': {
basic: 'Basic Usage',
autoPlay: 'Auto play',
autoPlay: 'Auto play, Ref pause and play',
muted: 'Initialize mute',
cover: 'Video cover poster settings',
inline: 'play inline',
Expand Down
15 changes: 13 additions & 2 deletions src/packages/video/demos/h5/demo2.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import React, { useState } from 'react'
import React, { useState, useRef } from 'react'
import { Cell, Video } from '@nutui/nutui-react'

const Demo2 = () => {
const [source, setSource] = useState({
const [source] = useState({
src: 'https://storage.360buyimg.com/nutui/video/video_NutUI.mp4',
type: 'video/mp4',
})
Expand All @@ -14,10 +14,21 @@ const Demo2 = () => {
const play = (elm: HTMLVideoElement) => console.log('play', elm)
const pause = (elm: HTMLVideoElement) => console.log('pause', elm)
const playend = (elm: HTMLVideoElement) => console.log('playend', elm)

const rootRef = useRef<HTMLVideoElement>(null)
setTimeout(() => {
rootRef?.current?.pause()
}, 2000)

setTimeout(() => {
rootRef?.current?.play()
}, 4000)

Comment on lines +18 to +25
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

建议优化定时器实现

当前实现存在以下几个问题:

  1. 没有清理定时器
  2. 硬编码的时间值不易维护
  3. 缺少错误处理

建议按照以下方式重构:

+const PAUSE_DELAY = 2000;
+const PLAY_DELAY = 4000;
 const rootRef = useRef<HTMLVideoElement>(null);
+
+useEffect(() => {
+  const pauseTimer = setTimeout(() => {
+    try {
+      rootRef?.current?.pause();
+    } catch (error) {
+      console.error('视频暂停失败:', error);
+    }
+  }, PAUSE_DELAY);
 
-setTimeout(() => {
-  rootRef?.current?.pause()
-}, 2000)
+  const playTimer = setTimeout(() => {
+    try {
+      rootRef?.current?.play();
+    } catch (error) {
+      console.error('视频播放失败:', error);
+    }
+  }, PLAY_DELAY);
 
-setTimeout(() => {
-  rootRef?.current?.play()
-}, 4000)
+  return () => {
+    clearTimeout(pauseTimer);
+    clearTimeout(playTimer);
+  };
+}, []);
📝 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
const rootRef = useRef<HTMLVideoElement>(null)
setTimeout(() => {
rootRef?.current?.pause()
}, 2000)
setTimeout(() => {
rootRef?.current?.play()
}, 4000)
const PAUSE_DELAY = 2000;
const PLAY_DELAY = 4000;
const rootRef = useRef<HTMLVideoElement>(null);
useEffect(() => {
const pauseTimer = setTimeout(() => {
try {
rootRef?.current?.pause();
} catch (error) {
console.error('视频暂停失败:', error);
}
}, PAUSE_DELAY);
const playTimer = setTimeout(() => {
try {
rootRef?.current?.play();
} catch (error) {
console.error('视频播放失败:', error);
}
}, PLAY_DELAY);
return () => {
clearTimeout(pauseTimer);
clearTimeout(playTimer);
};
}, []);

return (
<>
<Cell style={{ padding: '0' }}>
<Video
ref={rootRef}
source={source}
options={options}
onPlay={play}
Expand Down
7 changes: 7 additions & 0 deletions src/packages/video/doc.en-US.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,10 @@ Reset the video when the video address changes
| onPlay | play event | `(element: HTMLVideoElement) => void` | `-` |
| onPause | pause event | `(element: HTMLVideoElement) => void` | `-` |
| onPlayEnd | Playback completion callback | `(element: HTMLVideoElement) => void` | `-` |

### Ref

| Name | Description | Arguments |
| --- | --- | --- |
| play | play | `-` |
| pause | pause | `-` |
7 changes: 7 additions & 0 deletions src/packages/video/doc.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,10 @@ playsinline 属性设置移动端视频行内播放,阻止新打开页面播
| onPlay | 播放 | `(element: HTMLVideoElement) => void` | `-` |
| onPause | 暂停 | `(element: HTMLVideoElement) => void` | `-` |
| onPlayEnd | 播放完成回调 | `(element: HTMLVideoElement) => void` | `-` |

### Ref

| 方法名 | 说明 | 参数 |
| --- | --- | --- |
| play | 播放 | `-` |
| pause | 暂停 | `-` |
7 changes: 7 additions & 0 deletions src/packages/video/doc.zh-TW.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,10 @@ playsinline 屬性設置移動端視頻行內播放,阻止新打開頁面播
| onPlay | 播放 | `(element: HTMLVideoElement) => void` | `-` |
| onPause | 暫停 | `(element: HTMLVideoElement) => void` | `-` |
| onPlayEnd | 播放完成回調 | `(element: HTMLVideoElement) => void` | `-` |

### Ref

| 方法名 | 說明 | 參數 |
| --- | --- | --- |
| play | 播放 | `-` |
| pause | 暫停 | `-` |
26 changes: 22 additions & 4 deletions src/packages/video/video.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useEffect, useRef, FunctionComponent } from 'react'
import React, { useEffect, useRef } from 'react'
import classNames from 'classnames'
import { BasicComponent, ComponentDefaults } from '@/utils/typings'

Expand Down Expand Up @@ -36,11 +36,17 @@ const defaultProps = {
},
} as VideoProps

export type VideoRef = {
pause: () => void
play: () => void
}

const classPrefix = `nut-video`
export const Video: FunctionComponent<
export const Video = React.forwardRef<
VideoRef,
Partial<VideoProps> &
Omit<React.HTMLAttributes<HTMLDivElement>, 'onPause' | 'onPlay'>
> = (props) => {
>((props, ref) => {
const {
children,
source,
Expand Down Expand Up @@ -88,6 +94,18 @@ export const Video: FunctionComponent<
}
}

const pause = () => {
rootRef?.current?.pause()
}
const play = () => {
rootRef?.current?.play()
}

Comment on lines +97 to +102
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

建议增加错误处理!

playpause 方法应该添加错误处理,因为 HTMLVideoElement 的这些方法会返回 Promise。

建议修改实现如下:

-  const pause = () => {
-    rootRef?.current?.pause()
-  }
-  const play = () => {
-    rootRef?.current?.play()
-  }
+  const pause = async () => {
+    try {
+      await rootRef?.current?.pause()
+    } catch (error) {
+      console.error('视频暂停失败:', error)
+    }
+  }
+  const play = async () => {
+    try {
+      await rootRef?.current?.play()
+    } catch (error) {
+      console.error('视频播放失败:', error)
+    }
+  }
📝 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
const pause = () => {
rootRef?.current?.pause()
}
const play = () => {
rootRef?.current?.play()
}
const pause = async () => {
try {
await rootRef?.current?.pause()
} catch (error) {
console.error('视频暂停失败:', error)
}
}
const play = async () => {
try {
await rootRef?.current?.play()
} catch (error) {
console.error('视频播放失败:', error)
}
}
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 98-99: src/packages/video/video.tsx#L98-L99
Added lines #L98 - L99 were not covered by tests


[warning] 101-102: src/packages/video/video.tsx#L101-L102
Added lines #L101 - L102 were not covered by tests

React.useImperativeHandle(ref, () => ({
pause,
play,
}))

Comment on lines +97 to +107
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

需要添加单元测试!

新增的 ref 方法缺少测试覆盖,建议添加相应的单元测试用例。

建议添加以下测试场景:

  1. 测试 play 方法调用
  2. 测试 pause 方法调用
  3. 测试错误处理场景

需要我帮您生成测试用例代码吗?

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 98-99: src/packages/video/video.tsx#L98-L99
Added lines #L98 - L99 were not covered by tests


[warning] 101-102: src/packages/video/video.tsx#L101-L102
Added lines #L101 - L102 were not covered by tests


[warning] 105-106: src/packages/video/video.tsx#L105-L106
Added lines #L105 - L106 were not covered by tests

return (
<div className={classes} {...restProps}>
<video
Expand All @@ -105,6 +123,6 @@ export const Video: FunctionComponent<
</video>
</div>
)
}
})

Video.displayName = 'NutVideo'
Loading