Skip to content

Zoom icons 596 - #619

Merged
Dananji merged 7 commits into
mainfrom
zoom-icons-596
Sep 4, 2024
Merged

Zoom icons 596#619
Dananji merged 7 commits into
mainfrom
zoom-icons-596

Conversation

@Dananji

@Dananji Dananji commented Aug 12, 2024

Copy link
Copy Markdown
Collaborator

Related issue: #596

Included in this PR:

  • Re-arrange the player controls to provide more space for the controls when the UI is zoomed-in
  • Set player's min-width so that, all player controls are displayed without overflowing when the UI is zoomed-in

Screenshot 2024-08-12 at 2 27 16 PM

@elynema

elynema commented Aug 13, 2024

Copy link
Copy Markdown

@Dananji Looking at the screenshot, it looks like it's showing the progress bar above the other player controls to create space for the progress bar. I think this looks ok just from the screenshot (although I do notice that the icons on the second row are not vertically aligned).

We'll have to test this out once committed to see how we feel about the min-width.

@elynema

elynema commented Aug 13, 2024

Copy link
Copy Markdown

@joncameron You ok with moving the progress bar to a separate line once screen width starts to decrease.

@joncameron

Copy link
Copy Markdown
Collaborator

Yep, I'm good with this. @Dananji please go ahead and update the PR from draft so we can merge it in and play around with the change.

@Dananji

Dananji commented Aug 14, 2024

Copy link
Copy Markdown
Collaborator Author

I did not test this out in mobile devices as part of the work I did. So, I will test in mobile devices to make sure everything works as expected and make any changes as needed.
And fix vertical alignment of the icons.

@Dananji

Dananji commented Aug 14, 2024

Copy link
Copy Markdown
Collaborator Author

Changes made to controls in last commit;

Volume panel change in video player;
Screenshot 2024-08-14 at 2 11 27 PM

Volume panel change in audio player;
Screenshot 2024-08-14 at 2 12 21 PM

@Dananji
Dananji marked this pull request as ready for review August 14, 2024 18:13

@cjcolvar cjcolvar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good overall! I just had one question.

Comment thread src/components/MediaPlayer/VideoJS/components/js/VideoJSNextButton.js Outdated
@elynema

elynema commented Aug 23, 2024

Copy link
Copy Markdown

@joncameron Dananji changed the volume control so that it would not expand upward over top of the progress bar in these situations. Is that ok with you for first pass and we can try it out once this gets to QA?

@Dananji Did you consider using the horizontal volume slider like we use for audio only items?

@Dananji

Dananji commented Aug 23, 2024

Copy link
Copy Markdown
Collaborator Author

Yes, I did. And it looked a bit odd with the empty space on top and bottom for me. I think this wasn't noticeable before with the audio player because the control bar height was smaller then. In this work I increased the control bar height a little bit to make space.
If the horizontal volume panel with the mute toggle is preferred I can make that change for both audio and video player instances.
The only limitation with this design is that we can't use the vertical volume panel for video like before.

Comment thread src/components/MediaPlayer/VideoJS/videojs-theme.scss Outdated
@Dananji
Dananji requested a review from cjcolvar September 4, 2024 17:30

@cjcolvar cjcolvar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@Dananji
Dananji merged commit bc10eca into main Sep 4, 2024
@Dananji
Dananji deleted the zoom-icons-596 branch September 4, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants