Skip to content

Commit ed6eb50

Browse files
kduncanhsuDuncan Hsu
andauthored
refactor(video-player-v2): gate time view dropdown behind split (#1695)
* refactor(video-player-v2): gate time view dropdown behind split * test(video): update test syntax to be more clear --------- Co-authored-by: Duncan Hsu <kduncanhsu@box.com>
1 parent b84fd34 commit ed6eb50

9 files changed

Lines changed: 93 additions & 45 deletions

File tree

src/lib/viewers/controls/media/FilmstripV2.tsx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import React from 'react';
22
import classNames from 'classnames';
33
import formatTimecode from '../../media/formatTimecode';
4-
import { DEFAULT_FPS } from '../../media/videoFps';
4+
import { formatTime } from './DurationLabels';
55
import './FilmstripV2.scss';
66

77
const FILMSTRIP_FRAMES_PER_ROW = 100;
@@ -22,7 +22,7 @@ export type Props = {
2222

2323
export default function FilmstripV2({
2424
aspectRatio = 0,
25-
fps = DEFAULT_FPS,
25+
fps,
2626
imageUrl = '',
2727
interval = 1,
2828
isShown,
@@ -84,7 +84,7 @@ export default function FilmstripV2({
8484
)}
8585
</div>
8686
<div className="bp-FilmstripV2-time" data-testid="bp-FilmstripV2-time">
87-
{formatTimecode(time, fps)}
87+
{fps ? formatTimecode(time, fps) : formatTime(time)}
8888
</div>
8989
</div>
9090
);

src/lib/viewers/controls/media/TimestampControl.scss

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,11 @@
66
align-items: center;
77
}
88

9-
.bp-TimestampControl-button {
10-
@include bp-Control--fade;
11-
@include bp-Control--outline;
12-
9+
.bp-TimestampControl-button,
10+
.bp-TimestampControl-static {
1311
display: flex;
1412
gap: 4px;
1513
align-items: center;
16-
margin: 0;
1714
padding: 6px 12px;
1815
color: $white;
1916
font-weight: normal;
@@ -22,11 +19,18 @@
2219
line-height: 20px;
2320
letter-spacing: .3px;
2421
white-space: nowrap;
22+
user-select: none;
23+
}
24+
25+
.bp-TimestampControl-button {
26+
@include bp-Control--fade;
27+
@include bp-Control--outline;
28+
29+
margin: 0;
2530
background: transparent;
2631
border: 0;
2732
border-radius: $bp-controls-button-radius;
2833
cursor: pointer;
29-
user-select: none;
3034

3135
&:focus:not(:disabled),
3236
&.bp-is-open {

src/lib/viewers/controls/media/TimestampControl.tsx

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ export type Props = {
2020
currentTime?: number;
2121
durationTime?: number;
2222
fps?: number;
23+
canChangeTimeFormat?: boolean;
2324
isNarrowWidth?: boolean;
2425
mediaEl?: HTMLVideoElement | null;
2526
};
@@ -46,6 +47,7 @@ export default function TimestampControl({
4647
currentTime = 0,
4748
durationTime = 0,
4849
fps = DEFAULT_FPS,
50+
canChangeTimeFormat = false,
4951
isNarrowWidth = false,
5052
mediaEl,
5153
}: Props): JSX.Element {
@@ -138,6 +140,22 @@ export default function TimestampControl({
138140
handleSelect(selectedFormat);
139141
};
140142

143+
if (!canChangeTimeFormat) {
144+
return (
145+
<div className="bp-TimestampControl" data-testid="bp-TimestampControl">
146+
<span className="bp-TimestampControl-static" data-testid="bp-TimestampControl-static">
147+
<span className="bp-TimestampControl-current">{formatTime(currentTime)}</span>
148+
{!isNarrowWidth && (
149+
<>
150+
<span className="bp-TimestampControl-separator">/</span>
151+
<span className="bp-TimestampControl-duration">{formatTime(durationTime)}</span>
152+
</>
153+
)}
154+
</span>
155+
</div>
156+
);
157+
}
158+
141159
return (
142160
<div ref={containerElRef} className="bp-TimestampControl" data-testid="bp-TimestampControl">
143161
<button

src/lib/viewers/controls/media/__tests__/FilmstripV2-test.tsx

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,18 +55,24 @@ describe('FilmstripV2', () => {
5555
});
5656
});
5757

58-
describe('timecode', () => {
59-
test('should display formatted time', () => {
58+
describe('time display', () => {
59+
test('should display standard time when fps is not provided', () => {
6060
render(<FilmstripV2 time={65} />);
6161
const timeEl = screen.getByTestId('bp-FilmstripV2-time');
6262
expect(timeEl).toHaveTextContent('1:05');
6363
});
6464

65-
test('should display 0:00 for time 0', () => {
65+
test('should display 0:00 for time 0 when fps is not provided', () => {
6666
render(<FilmstripV2 time={0} />);
6767
const timeEl = screen.getByTestId('bp-FilmstripV2-time');
6868
expect(timeEl).toHaveTextContent('0:00');
6969
});
70+
71+
test('should display timecode when fps is provided', () => {
72+
render(<FilmstripV2 fps={30} time={61.5} />);
73+
const timeEl = screen.getByTestId('bp-FilmstripV2-time');
74+
expect(timeEl).toHaveTextContent('00:01:01:15');
75+
});
7076
});
7177

7278
describe('loading state', () => {

src/lib/viewers/controls/media/__tests__/TimestampControl-test.tsx

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,16 @@ describe('TimestampControl', () => {
2020
});
2121

2222
const getWrapper = (props = {}) =>
23-
render(<TimestampControl currentTime={65} durationTime={120} fps={24} mediaEl={videoEl} {...props} />);
23+
render(
24+
<TimestampControl
25+
canChangeTimeFormat
26+
currentTime={65}
27+
durationTime={120}
28+
fps={24}
29+
mediaEl={videoEl}
30+
{...props}
31+
/>,
32+
);
2433

2534
const getButton = () => screen.getByTestId('bp-TimestampControl-button');
2635

@@ -43,6 +52,23 @@ describe('TimestampControl', () => {
4352
});
4453
});
4554

55+
describe('when canChangeTimeFormat is false', () => {
56+
test('should render static standard time without a dropdown', () => {
57+
getWrapper({ canChangeTimeFormat: false });
58+
59+
expect(screen.getByTestId('bp-TimestampControl-static')).toBeInTheDocument();
60+
expect(screen.getByTestId('bp-TimestampControl-static')).toHaveTextContent('1:05/2:00');
61+
expect(screen.queryByTestId('bp-TimestampControl-button')).not.toBeInTheDocument();
62+
});
63+
64+
test('should render only current time when isNarrowWidth is true', () => {
65+
getWrapper({ canChangeTimeFormat: false, isNarrowWidth: true });
66+
67+
expect(screen.getByTestId('bp-TimestampControl-static')).toHaveTextContent('1:05');
68+
expect(screen.getByTestId('bp-TimestampControl-static')).not.toHaveTextContent('/');
69+
});
70+
});
71+
4672
describe('dropdown', () => {
4773
test('should open the flyout with 3 format options when clicked', async () => {
4874
const user = userEvent.setup();

src/lib/viewers/media/DashViewer.js

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1214,16 +1214,11 @@ class DashViewer extends VideoBaseViewer {
12141214

12151215
/**
12161216
* Returns the FPS to use for frame-based UI (timecode/frame formats, scrubber step).
1217-
* V2 player always has an FPS (manifest value or 24fps fallback); the V1 player
1218-
* only exposes it when frame stepping is enabled and the manifest provides it.
1217+
* Only exposes FPS when frame stepping is enabled and the manifest provides it.
12191218
*
12201219
* @return {number|undefined} Frames per second
12211220
*/
12221221
getFps() {
1223-
if (this.isVideoPlayerV2) {
1224-
return getVideoFps(this.player);
1225-
}
1226-
12271222
return this.featureEnabled('frameStep.enabled') && isFpsAvailable(this.player)
12281223
? getVideoFps(this.player)
12291224
: undefined;
@@ -1280,6 +1275,7 @@ class DashViewer extends VideoBaseViewer {
12801275
audioTracks: this.audioTracks,
12811276
autoplay: this.isAutoplayEnabled(),
12821277
bufferedRange: this.mediaEl.buffered,
1278+
canChangeTimeFormat: this.featureEnabled('frameStep.enabled'),
12831279
currentTime: this.mediaEl.currentTime,
12841280
durationTime: this.mediaEl.duration,
12851281
experiences: this.experiences,

src/lib/viewers/media/VideoControlsV2.tsx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ export type Props = DurationLabelsProps &
2626
TimeControlsProps &
2727
VolumeControlsProps & {
2828
fps?: number;
29+
canChangeTimeFormat?: boolean;
2930
annotationColor?: string;
3031
annotationMode?: AnnotationMode;
3132
experiences?: Experiences;
@@ -57,6 +58,7 @@ export default function VideoControlsV2({
5758
filmstripInterval,
5859
filmstripUrl,
5960
fps,
61+
canChangeTimeFormat,
6062
guide,
6163
hasDrawing,
6264
hasRegion,
@@ -152,6 +154,7 @@ export default function VideoControlsV2({
152154
</>
153155
)}
154156
<TimestampControl
157+
canChangeTimeFormat={canChangeTimeFormat}
155158
currentTime={currentTime}
156159
durationTime={durationTime}
157160
fps={fps}

src/lib/viewers/media/__tests__/DashViewer-test.js

Lines changed: 3 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1884,27 +1884,7 @@ describe('lib/viewers/media/DashViewer', () => {
18841884
});
18851885

18861886
describe('getFps()', () => {
1887-
test('should return manifest fps when videoPlayerV2 is enabled', () => {
1888-
dash.player = {
1889-
getVariantTracks: () => [{ frameRate: 30, active: true }],
1890-
destroy: jest.fn(),
1891-
};
1892-
dash.isVideoPlayerV2 = true;
1893-
1894-
expect(dash.getFps()).toBe(30);
1895-
});
1896-
1897-
test('should return 24fps fallback when videoPlayerV2 is enabled but manifest fps is unavailable', () => {
1898-
dash.player = {
1899-
getVariantTracks: () => [{ active: true }],
1900-
destroy: jest.fn(),
1901-
};
1902-
dash.isVideoPlayerV2 = true;
1903-
1904-
expect(dash.getFps()).toBe(24);
1905-
});
1906-
1907-
test('should return manifest fps when only frameStep is enabled and fps is available', () => {
1887+
test('should return manifest fps when frameStep is enabled and fps is available', () => {
19081888
dash.player = {
19091889
getVariantTracks: () => [{ frameRate: 30, active: true }],
19101890
destroy: jest.fn(),
@@ -1914,7 +1894,7 @@ describe('lib/viewers/media/DashViewer', () => {
19141894
expect(dash.getFps()).toBe(30);
19151895
});
19161896

1917-
test('should return undefined when only frameStep is enabled and fps is unavailable', () => {
1897+
test('should return undefined when frameStep is enabled but fps is unavailable', () => {
19181898
dash.player = {
19191899
getVariantTracks: () => [{ active: true }],
19201900
destroy: jest.fn(),
@@ -1924,7 +1904,7 @@ describe('lib/viewers/media/DashViewer', () => {
19241904
expect(dash.getFps()).toBeUndefined();
19251905
});
19261906

1927-
test('should return undefined when neither feature is enabled', () => {
1907+
test('should return undefined when frameStep is not enabled', () => {
19281908
dash.player = {
19291909
getVariantTracks: () => [{ frameRate: 30, active: true }],
19301910
destroy: jest.fn(),

src/lib/viewers/media/__tests__/VideoControlsV2-test.tsx

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,22 +84,37 @@ describe('VideoControlsV2', () => {
8484
});
8585

8686
describe('timestamp', () => {
87-
test('should show current / total time when not narrow', () => {
87+
test('should show static standard time when canChangeTimeFormat is false', () => {
8888
render(<VideoControlsV2 {...defaultProps} currentTime={65} durationTime={120} />);
89+
const timestamp = screen.getByTestId('bp-TimestampControl-static');
90+
expect(timestamp).toHaveTextContent('1:05/2:00');
91+
expect(screen.queryByTestId('bp-TimestampControl-button')).not.toBeInTheDocument();
92+
});
93+
94+
test('should show current / total time when not narrow', () => {
95+
render(<VideoControlsV2 {...defaultProps} canChangeTimeFormat currentTime={65} durationTime={120} />);
8996
const timestamp = screen.getByTestId('bp-TimestampControl-button');
9097
expect(timestamp).toHaveTextContent('1:05/2:00');
9198
});
9299

93100
test('should show only current time when narrow', () => {
94-
render(<VideoControlsV2 {...defaultProps} currentTime={65} durationTime={120} isNarrowVideo />);
101+
render(
102+
<VideoControlsV2
103+
{...defaultProps}
104+
canChangeTimeFormat
105+
currentTime={65}
106+
durationTime={120}
107+
isNarrowVideo
108+
/>,
109+
);
95110
const timestamp = screen.getByTestId('bp-TimestampControl-button');
96111
expect(timestamp).toHaveTextContent('1:05');
97112
expect(timestamp).not.toHaveTextContent('/');
98113
});
99114

100115
test('should open the time format dropdown when timestamp is clicked', async () => {
101116
const user = userEvent.setup();
102-
render(<VideoControlsV2 {...defaultProps} currentTime={65} durationTime={120} />);
117+
render(<VideoControlsV2 {...defaultProps} canChangeTimeFormat currentTime={65} durationTime={120} />);
103118

104119
await user.click(screen.getByTestId('bp-TimestampControl-button'));
105120
expect(screen.getByTestId('bp-TimestampControl-flyout')).toBeInTheDocument();

0 commit comments

Comments
 (0)