-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Conversation
Holding off on this until we lock down for 5.0. |
Should we more formally name this? "button separator", "insert point", er something? |
After conversation:
|
@heff updated to reflect our most recent conversation. |
5891acc
to
6f535de
Compare
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 |
6f535de
to
070290f
Compare
*/ | ||
class InsertionPointSpacer extends Spacer { | ||
buildCSSClass() { | ||
return `vjs-insertion-point-spacer-control ${super.buildCSSClass}`; |
There was a problem hiding this comment.
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.
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. |
CustomControlInsertPoint? We should also add a method to the control bar so you can just do |
8ca5fc4
to
9a016f2
Compare
control bar layouts
split spacer into two files and converted to es6 insertion-point-spacer -> custom-control-spacer
9a016f2
to
4f52a47
Compare
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: