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(ui): Add Loop, PIP, Cast, AirPlay buttons to control panel #3255

Merged
merged 9 commits into from
Mar 30, 2021

Conversation

nbcl
Copy link
Contributor

@nbcl nbcl commented Mar 19, 2021

Description

The present pull request provides a partial solution to #2676 (Add overflowMenuButtons to the UI bar) by making the Picture-in-picture, Loop, Cast and AirPlay buttons available in the UI control panel.

The final result is achieved using the CSS display none approach suggested by @joeyparrish, and looks as follows:

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change includes a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code
  • I have made the corresponding changes to the documentation
  • My changes generate no new warnings

The new version of the picture-in-picture button is now available to be placed in the control panel of the UI.

Issue google#2676
The new version of the cast button is now available to be placed in the control panel of the UI.
The new version of the AirPlay button is now available to be placed in the control panel of the UI.
The new version of the loop button is now available to be placed in the control panel of the UI.
Documentation is updated to include the picture_in_picture, loop, airplay and cast buttons in the control panel.
Adds a line that was available in the overflow menu list, but was missing in the control panel list.
Removes an incorrect space in ui/loop_button.js and adds a missing one in ui/airplay_button.js.
@joeyparrish
Copy link
Member

To correctly follow the user experience intentions of Shaka Player, I would like to know which pair combination of the following icons would be ideal to represent the "loop on" and "loop off" states, as there is no "loop_off" icon in the library.

How about "repeat" and "repeat_on" ?

image

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

It looks great otherwise. I really only had this one complaint. Nice job!

@@ -170,6 +170,11 @@
}
}

/* Buttons hide certain items if they are found inside the control panel */
.shaka-controls-button-panel .display-none-if-on-control {
Copy link
Member

Choose a reason for hiding this comment

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

This class name feels weird to me. I think "shaka-overflow-menu-only" might be a better name, but I'd be happy to debate it if you disagree.

As the saying goes, there's really only one hard problem in computer science: naming things, and off-by-one errors. 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am ashamed to admit how much time it took me to understand that saying the first time I read it.

Oh well, I changed it now, let us see what other weird variable names I come up with next. 😄

@nbcl
Copy link
Contributor Author

nbcl commented Mar 20, 2021

Alright, I am marking this pull request as ready for review! 🎉

@nbcl nbcl marked this pull request as ready for review March 20, 2021 22:01
Loop "On" and "Off" buttons were "loop" and "sync_disabled". They are now changed to "repeat" and "repeat_on".
The CSS class that hides certain items if they are found inside the control panel was "display-none-if-on-control". This commit changes it to "shaka-overflow-menu-only".
@shaka-bot
Copy link
Collaborator

All tests passed!

@joeyparrish joeyparrish merged commit 4f0ded7 into shaka-project:master Mar 30, 2021
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants