-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
style(_progress): Provide padding for the vjs-progress-holder
while…
#3658
Conversation
… allowing it to resize on hover. Thereby containing the `MouseTimeDisplay` to the `vjs-progress-holder` instead of it bleeding outside. NOTE: Guessing that the margin was put in place to provide some spacing between the slider and the surrounding controls. Because the slider itself, or rather, `vjs-progress-holder` becomes larger on hover, we cannot have the "padding" also be responsive on the same element.
@@ -3,7 +3,6 @@ | |||
position: relative; | |||
cursor: pointer; | |||
padding: 0; | |||
margin: 0 0.45em 0 0.45em; |
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.
does the volume slider still look ok?
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.
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.
It seems like only this removal is necessary to fix the tooltip issue.
@@ -15,6 +15,7 @@ | |||
.video-js .vjs-progress-control { | |||
@include flex(auto); | |||
@include display-flex(center); | |||
margin: 0 0.45em 0 0.45em; |
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.
Looks like this would break the Brightcove skin by making the tooltips go out of bounds with the keepTooltipsInside
. It probably would also break skins like https://github.com/mmcc/videojs-skin-twitchy.
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.
I would just get rid of this line as it seems to be unnecessary.
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.
If there isn't any margin added it makes it very difficult to access the surrounding components because there is only 1px between the volume and the seekbar at position 0.
I didn't quite follow the comment.
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.
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.
It would seem that the Brightcove skin might need to include that in the source if it utilizes a fullscreen SeekBar
?
Should video.js accommodate fullscreen SeekBar
in its implementation?
Not sure best way to move forward on this.
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.
it isnt just in fullscreen that's the issue. It gets cut off by the overflow. The videojs-skin-twichy would also have a similar issue.
But basically it means that we cannot make this change as is because it would break this usecase.
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.
Hmm, I am not sure how we would go about fixing this issue if the default styles directly conflict with other skins.
Maybe the Brightcove skin is doing something different (Haven't tested).
The default is embedded in the control bar, but the other is not...? I guess its kind of chicken/egg'ish.
If it makes sense for the default implementation to be the primary one, then the other skins would need to undo these changes?
The twitchy skin implements https://github.com/mmcc/videojs-skin-twitchy/blob/master/src/components/_slider.scss#L6 (which removes the inline SeekBar
margin). It would also need to remove the .vjs-progress-control
ones too (if they were added).
non-modified twitchy skin (has the same out of bounds issue though—might be a different issue?—it breaks regardless of keepTooltipsInside
)
Not saying that this should be the case, just that the styles are conflicting—And its weird for the default implementation to have a MouseTimeDisplay
that goes out of bounds, especially on the home page :p.
Maybe I should ping in channel and get other ideas?
@@ -3,7 +3,6 @@ | |||
position: relative; | |||
cursor: pointer; | |||
padding: 0; | |||
margin: 0 0.45em 0 0.45em; |
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.
It seems like only this removal is necessary to fix the tooltip issue.
I think I'm going to close this PR. Not sure exactly how to fix this without potentially breaking users. In videojs 6.0, we're going to transition to only having one kind of progress with mouse tooltips and we can make the fix there. |
Description
Fixes #3645. Looks like it doesn't effect
volume-bar
, which is the only other dependent ofSlider
, because it manages its own margins. SEE: https://github.com/videojs/video.js/blob/master/src/css/components/_volume.scss#L28Specific Changes proposed
Requirements Checklist
… allowing it to resize on hover. Thereby containing the
MouseTimeDisplay
to thevjs-progress-holder
instead of it bleeding outside.NOTE: Guessing that the margin was put in place to provide some spacing between the slider and the surrounding controls. Because the slider itself, or rather,
vjs-progress-holder
becomes larger on hover, we cannot have the "padding" also be responsive on the same element.