feat(video): create Video component #72

kaseyvee · 2024-04-10T14:56:08Z

Changes

create video component
refactor controls to work with video

Screenshots/Videos

Designs

Desktop

Mobile

Storybook

Light mode

Default

With captions

With poster

Error

Mobile

Dark mode

Default

Mobile

Videos

Desktop Fullscreen
https://github.com/rustic-ai/ui-components/assets/111031789/e0ad61a1-f119-4c3e-adb9-068c3fe5f0f7

Desktop Picture-in-picture
https://github.com/rustic-ai/ui-components/assets/111031789/2f22fe6d-90b7-4c4e-b7e4-a2109d7f5c26

Mobile Fullscreen
https://github.com/rustic-ai/ui-components/assets/111031789/36a9f7fd-4652-4d72-be52-a6e4e32f56b8

Shiti

I'm thinking we should split the controls file into 3 - mediaIcons component, common(controls common to audio and video) and videocontrols. what do you think ?

Shiti · 2024-04-10T15:20:00Z

src/components/media/controls/controls.tsx

+    | 'pictureInPicture'
+    | 'pictureInPictureExit'
+    | 'fullscreen'
+    | 'fullscreenExit'


I think it would be good to add the captions ones here too

Shiti · 2024-04-10T15:21:42Z

src/components/media/controls/controls.tsx

+    | 'fullscreen'
+    | 'fullscreenExit'
  className?: string
+  color?: string


why do we need to pass the color as prop? can we not set it to 'primary' always ?

The colours change based on the component as well as the viewport, so I was trying to think of a way to control this.

No, the colors should not change by viewport. Please set this to primary

Shiti

Please update the commit message to clearly indicate what is being changed. Also, for tests, I would recommend using a local video file as that would help speed things up

lyjeileen · 2024-04-10T16:32:34Z

Could you hide the control bar when the mouse has not moved for a few seconds?

lyjeileen · 2024-04-10T16:43:45Z

src/components/types.ts

  transcript?: string
 }
+
+export interface AudioFormat extends MediaFormat {}


Question: Is it necessary to add AudioFormat when it's exactly same as MediaFormat? Do we want to introduce a new type just for the semantic reason?

Semantics for clarity but I'm also thinking in the future there could possibly be audio-only fields.

kaseyvee · 2024-04-10T17:39:35Z

I'm thinking we should split the controls file into 3 - mediaIcons component, common(controls common to audio and video) and videocontrols. what do you think ?

@Shiti This would make it easier to maintain. Will still have to think about how the controls in the settings menu will work.

Shiti · 2024-04-10T21:47:40Z

src/components/media/controls/commonControls.tsx

-  const Icon = props.isTranscriptShown
-    ? KeyboardArrowUpIcon
-    : KeyboardArrowDownIcon
+export function CaptionsToggle(props: Toggle) {


Suggested change

export function CaptionsToggle(props: Toggle) {

export function ToggleCaptionsButton(props: Toggle) {

I dont think we need to create a component for these 2 lines. Its not adding any value

I figured since we're using it in both the Sound and Video components for now until the Video settings menu design flow was finalized, since it seems like playback rate, captions, etc will later be handled there.

Still, in other cases, where you have defined a component for button, there is additional logic, whereas here there is none.

src/components/media/controls/commonControls.tsx

Shiti · 2024-04-11T15:49:10Z

src/components/media/controls/commonControls.tsx

-  const Icon = props.isTranscriptShown
-    ? KeyboardArrowUpIcon
-    : KeyboardArrowDownIcon
+export function CaptionsToggle(props: Toggle) {


I dont think we need to create a component for these 2 lines. Its not adding any value

src/components/media/controls/commonControls.tsx

src/components/types.ts

Laurendragonscale · 2024-04-11T22:01:54Z

We should consider adding a lot more contrast behind the video controls. Maybe even a solid colour like this:

lyjeileen · 2024-04-11T23:39:45Z

Hi! I found a bug in the mobile version. Here are the steps to reproduce it:

Click play button (It switched to the full screen mode)
Click the full screen toggle (It switched back to the small screen)
Click play button again => Nothing happened
Click play button again (It switched to the full screen mode)

src/components/media/audio/sound.cy.tsx

src/components/video/video.tsx

lyjeileen · 2024-04-12T00:10:34Z

Should the video folder be in the media folder?

kaseyvee · 2024-04-12T00:37:13Z

Hi! I found a bug in the mobile version. Here are the steps to reproduce it:

Click play button (It switched to the full screen mode)

Click the full screen toggle (It switched back to the small screen)

Click play button again => Nothing happened

Click play button again (It switched to the full screen mode)

@lyjeileen Addressed. Initially spoke with @RenataDzotova about making this immediately go to fullscreen on mobile since it's not obvious you have to go to fullscreen for the controls. Maybe we can address this in the future, so I've updated to be back to the original design. Clicking play now simply plays the video without going fullscreen and added back the fullscreen button.

src/components/media/controls/commonControls.tsx

Shiti · 2024-04-12T19:20:00Z

@kaseyvee when transcript is shown in full screen mode, the other controls are not visible even on hover. We have to hide transcript for any other controls to be visible.

Shiti · 2024-04-12T19:25:35Z

When showing picture-in-picture, I think it should show the poster /first frame at the original location.

Shiti · 2024-04-12T19:29:31Z

src/components/media/audio/sound.tsx

    )
  }

  function renderCaptionsToggle() {


Suggested change

function renderCaptionsToggle() {

function renderCaptionsButton() {

Shiti · 2024-04-12T19:31:20Z

src/components/media/controls/commonControls.tsx


-export function PausePlayToggle(props: MediaControls) {
-  const [isPlaying, setIsPlaying] = useState(false)
+export function PlayOrPauseToggle(props: MediaControls) {


Suggested change

export function PlayOrPauseToggle(props: MediaControls) {

export function PlayOrPauseButton(props: MediaControls) {

Shiti · 2024-04-12T19:33:12Z

src/components/media/controls/videoControls.tsx

+  element: HTMLElement
+}
+
+export function PictureInPictureToggle(props: MediaControls) {


Suggested change

export function PictureInPictureToggle(props: MediaControls) {

export function PictureInPictureButton(props: MediaControls) {

Shiti · 2024-04-12T20:02:03Z

src/components/media/controls/videoControls.tsx

+  function handlePictureInPicture() {
+    if (isPictureInPicture) {
+      document.exitPictureInPicture()
+    } else {
+      videoElement.requestPictureInPicture()
+    }
+  }


Both document.exitPictureInPicture() and videoElement.requestPictureInPicture() return Promise. Use that instead of adding event handlers below.

I've left the onleavepictureinpicture handler for when users exit from the pip window.

Shiti · 2024-04-12T20:02:52Z

src/components/media/controls/videoControls.tsx

+      document.exitFullscreen()
+    } else {
+      props.element.requestFullscreen()
+    }


Please update this to handle the returned Promise.

kaseyvee · 2024-04-12T20:14:48Z

@kaseyvee when transcript is shown in full screen mode, the other controls are not visible even on hover. We have to hide transcript for any other controls to be visible.

@Shiti Not quite sure how the designs are meant to handle showing the transcript on fullscreen with the controls. I could make them appear like this with a max height of 25% viewport height:

@Laurendragonscale @RenataDzotova @lyjeileen What do you guys think?

Shiti · 2024-04-12T20:19:32Z

on desktop, the transcript comes below the controls so I would assume that the same happens on mobile as well.

src/components/media/controls/videoControls.tsx

src/components/media/video/video.tsx

Shiti · 2024-04-15T16:34:52Z

src/components/media/controls/commonControls.tsx

-      props.mediaElement.play()
-      setIsPlaying(true)
+      props.mediaElement.play().catch(() => {
+        props.onError('Failed to play the media. Please try again.')


You are catching the exception but not capturing any information regarding it.

Possible errors include:

NotAllowedError DOMException
Provided if the user agent (browser) or operating system doesn't allow playback of media in the current context or situation. The browser may require the user to explicitly start media playback by clicking a "play" button, for example because of a Permissions Policy.

NotSupportedError DOMException
Provided if the media source (which may be specified as a MediaStream, MediaSource, Blob, or File, for example) doesn't represent a supported media format.

In these scenarios, trying again will be of no use. The appropriate message should be indicated clearly so user understands what the issue is and why the media cannot be played.

Reference - https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/play

src/components/media/video/video.css

src/components/media/video/video.tsx

Co-authored-by: Shiti Saxena <[email protected]> Signed-off-by: kasey <[email protected]>

Shiti reviewed Apr 10, 2024

View reviewed changes

lyjeileen reviewed Apr 10, 2024

View reviewed changes

kaseyvee force-pushed the dev branch from 5d350ec to 58bc002 Compare April 10, 2024 17:51

Shiti reviewed Apr 10, 2024

View reviewed changes

kaseyvee force-pushed the dev branch from fec831f to ff05c75 Compare April 10, 2024 23:57

Shiti reviewed Apr 11, 2024

View reviewed changes

kaseyvee marked this pull request as ready for review April 11, 2024 18:23

kaseyvee force-pushed the dev branch 2 times, most recently from b1897f5 to bb98c0f Compare April 11, 2024 21:52

kaseyvee requested review from Shiti and lyjeileen April 11, 2024 23:23

lyjeileen reviewed Apr 12, 2024

View reviewed changes

src/components/media/audio/sound.cy.tsx Outdated Show resolved Hide resolved

src/components/video/video.tsx Outdated Show resolved Hide resolved

src/components/video/video.tsx Outdated Show resolved Hide resolved

lyjeileen approved these changes Apr 12, 2024

View reviewed changes

Shiti reviewed Apr 12, 2024

View reviewed changes

src/components/media/controls/commonControls.tsx Show resolved Hide resolved

Shiti requested changes Apr 12, 2024

View reviewed changes

kaseyvee requested a review from Shiti April 13, 2024 17:19

Shiti requested changes Apr 14, 2024

View reviewed changes

Shiti reviewed Apr 14, 2024

View reviewed changes

src/components/media/video/video.tsx Outdated Show resolved Hide resolved

kaseyvee requested a review from Shiti April 15, 2024 15:17

kaseyvee added 19 commits April 15, 2024 08:20

fix: remove captions toggle component

01d3fc2

test: update tests

361bcb2

fix: add video to exports

2905cd0

fix: visual bug when showing short transcripts on fullscreen

7461857

fix: add background colour to controls

813a4be

fix: bug when entering fullscreen on mobile

7d97130

refactor: replace missed backgroundColor with variable

fc6e818

refactor: move video folder to media

37c9891

fix: bug when switching between viewport sizes on fullscreen

69efd6c

test: update tests

edfa4d7

ci: make cypress run with chrome

ac60180

docs: add video in quickstart example

b52deca

refactor: move youtube back to original folder

5739f4b

refactor: add suggestions for handleVolumeChange

c493dfa

docs: add comments for event listeners

1afd19a

test: update mediaIconButton test attributes

93dbc3e

fix: add transcript and promise handling suggestions

a5fa502

test: add test for fullscreen error

b76f3b1

fix: add suggestions

c985ed3

kaseyvee force-pushed the dev branch from 4c989f9 to c985ed3 Compare April 15, 2024 15:21

Shiti reviewed Apr 15, 2024

View reviewed changes

src/components/media/video/video.css Outdated Show resolved Hide resolved

kaseyvee added 3 commits April 15, 2024 11:08

fix: update variable names and show error descriptions

e7fb9d7

feat: hide controls on mouse inactivity

acb808b

feat: handle keyboard user inactivity

608682a

Shiti requested changes Apr 15, 2024

View reviewed changes

src/components/media/video/video.tsx Outdated Show resolved Hide resolved

src/components/media/video/video.tsx Outdated Show resolved Hide resolved

src/components/media/video/video.tsx Outdated Show resolved Hide resolved

kaseyvee and others added 2 commits April 15, 2024 12:02

Apply suggestions from code review

1bcfbb9

Co-authored-by: Shiti Saxena <[email protected]> Signed-off-by: kasey <[email protected]>

docs: make captions story have smaller dimensions

e4c917d

Shiti approved these changes Apr 15, 2024

View reviewed changes

Shiti merged commit 1fbda4b into rustic-ai:main Apr 15, 2024

	export function CaptionsToggle(props: Toggle) {
	export function ToggleCaptionsButton(props: Toggle) {

	function renderCaptionsToggle() {
	function renderCaptionsButton() {

	export function PlayOrPauseToggle(props: MediaControls) {
	export function PlayOrPauseButton(props: MediaControls) {

	export function PictureInPictureToggle(props: MediaControls) {
	export function PictureInPictureButton(props: MediaControls) {

Uh oh!

feat(video): create Video component #72

feat(video): create Video component #72

Uh oh!

Conversation

kaseyvee commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Screenshots/Videos

Designs

Desktop

Mobile

Storybook

Light mode

Dark mode

Videos

Uh oh!

Shiti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shiti Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shiti left a comment

Choose a reason for hiding this comment

Uh oh!

lyjeileen commented Apr 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaseyvee commented Apr 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Laurendragonscale commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lyjeileen commented Apr 11, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lyjeileen commented Apr 12, 2024

Uh oh!

kaseyvee commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Shiti commented Apr 12, 2024

Uh oh!

Shiti commented Apr 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kaseyvee commented Apr 10, 2024 •

edited

Loading

Shiti Apr 10, 2024 •

edited

Loading

Laurendragonscale commented Apr 11, 2024 •

edited

Loading

kaseyvee commented Apr 12, 2024 •

edited

Loading

Shiti Apr 15, 2024 •

edited

Loading