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.

Sign up for GitHub

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

Jump to bottom

style(_progress): Provide padding for the vjs-progress-holder while… #3658

Closed
wants to merge 1 commit into from
Closed

style(_progress): Provide padding for the vjs-progress-holder while… #3658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/css/components/_progress.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
.video-js .vjs-progress-control {
@include flex(auto);
@include display-flex(center);
margin: 0 0.45em 0 0.45em;
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, you're right. There is a slight difference.
The issue is that with this line here, our tooltips look like this:
broken tooltip
notice how the right side is a bit cut off. Vs, this:
working tooltip

The position of the mouse marker in both those cases is correct but the tooltip isn't.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

@chemoish chemoish Oct 4, 2016 •
edited
Loading

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).

image
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?

min-width: 4em;
}

Expand Down
1 change: 0 additions & 1 deletion src/css/components/_slider.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
position: relative;
cursor: pointer;
padding: 0;
margin: 0 0.45em 0 0.45em;

Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

image

image

Copy link
Member

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.

@include background-color-with-alpha($secondary-background-color, $secondary-background-transparency);
}
Expand Down