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

Zoom icons 596 #619

Merged
merged 7 commits into from
Sep 4, 2024
Merged

Zoom icons 596 #619

merged 7 commits into from
Sep 4, 2024

Conversation

Dananji
Copy link
Collaborator

@Dananji Dananji commented Aug 12, 2024

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
Copy link

elynema commented Aug 13, 2024

@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
Copy link

elynema commented Aug 13, 2024

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

@joncameron
Copy link
Contributor

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
Copy link
Collaborator Author

Dananji commented Aug 14, 2024

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
Copy link
Collaborator Author

Dananji commented Aug 14, 2024

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
Copy link
Member

@cjcolvar cjcolvar left a comment

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.

@elynema
Copy link

elynema commented Aug 23, 2024

@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
Copy link
Collaborator Author

Dananji commented Aug 23, 2024

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.

@Dananji Dananji requested a review from cjcolvar September 4, 2024 17:30
Copy link
Member

@cjcolvar cjcolvar left a comment

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
2 checks passed
@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