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

Feature/control bar separator #1838

Merged
merged 2 commits into from
Apr 24, 2015
Merged

Conversation

mmcc
Copy link
Member

@mmcc mmcc commented Jan 30, 2015

I have this in my updated base styles branch for 5.0, but I'm trying to pull components out of that one and submit separate PRs so that's not too unwieldy when it comes time for review. Basically this serves a few purposes:

  1. Provide an injection point for plugins. Plugins can append any control bar elements they create after this separator and have reasonable assurance of where it will end up in the dom (which in my branch, is after the progress bar / time elements but before the playback rate menu button).
  2. Allow for styling in different situations. Right now the base styles branch has the progress bar fill all available space between buttons on the left and right sides of the control bar. If one were to move the progress bar to where it is in the current layout, there would need to be styling work done to fill the empty void and keep the elements to the left / right. With the separator, the separator can be set to auto width in the absence of the control bar and nothing else needs to be changed.

@heff
Copy link
Member

heff commented Feb 11, 2015

Holding off on this until we lock down for 5.0.

@heff
Copy link
Member

heff commented Feb 11, 2015

Should we more formally name this? "button separator", "insert point", er something?

@mmcc
Copy link
Member Author

mmcc commented Mar 5, 2015

After conversation:

  • Keep generalized separator
  • Add specific separator name to default control bar separator instance (.buttons-separator)

@mmcc
Copy link
Member Author

mmcc commented Mar 9, 2015

@heff updated to reflect our most recent conversation.

@mmcc mmcc mentioned this pull request Apr 2, 2015
@mmcc mmcc force-pushed the feature/control-bar-separator branch 4 times, most recently from 5891acc to 6f535de Compare April 23, 2015 23:37
@mmcc
Copy link
Member Author

mmcc commented Apr 23, 2015

I'd like to get this pulled in before I keep going on #1999, so can we get another pass? I decided to go with spacer instead of separator since it seems like it's more commonly used. The "insertion point" bit is still up for debate, but let's make a decision and finish this off.

@mmcc mmcc force-pushed the feature/control-bar-separator branch from 6f535de to 070290f Compare April 23, 2015 23:40
*/
class InsertionPointSpacer extends Spacer {
buildCSSClass() {
return `vjs-insertion-point-spacer-control ${super.buildCSSClass}`;
Copy link
Member

Choose a reason for hiding this comment

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

We don't actually need -control after every class. It started that way because I was expecting every control to inherit from a control class, but that didn't end up happening.

@heff
Copy link
Member

heff commented Apr 24, 2015

I don't see any problems with the code.

What would the code look like when someone uses this to add a button? That might help with the naming.

@heff
Copy link
Member

heff commented Apr 24, 2015

CustomControlInsertPoint?

We should also add a method to the control bar so you can just do player.controlBar.addCustomControl(something).

@mmcc mmcc force-pushed the feature/control-bar-separator branch from 8ca5fc4 to 9a016f2 Compare April 24, 2015 18:11
split spacer into two files and converted to es6

insertion-point-spacer  -> custom-control-spacer
@mmcc mmcc force-pushed the feature/control-bar-separator branch from 9a016f2 to 4f52a47 Compare April 24, 2015 18:21
mmcc added a commit that referenced this pull request Apr 24, 2015
@mmcc mmcc merged commit 232daae into videojs:master Apr 24, 2015
@mmcc mmcc deleted the feature/control-bar-separator branch April 24, 2015 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants