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: enable responsive controls on fullscreen #7098

Merged
merged 1 commit into from
Mar 23, 2021

Conversation

ipadilla4
Copy link
Contributor

@ipadilla4 ipadilla4 commented Feb 17, 2021

A client reported that control buttons are not positioned correctly when player is full-screened and screen is small.

Currently we are excluding our responsive controls on fullscreen and changes include removing that. This solves the issue for small screens. Also added !important rule to override BC skin properties and hide controls when necessary.

@gkatsev gkatsev added the needs: LGTM Needs one or more additional approvals label Mar 9, 2021
Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

This is good, I think, but we should look at our internal BC skin to see if we can change the specificity such that the !important is not necessary.

@misteroneill misteroneill removed the needs: LGTM Needs one or more additional approvals label Mar 9, 2021
@gkatsev gkatsev changed the title Enable responsive controls on fullscreen feat: enable responsive controls on fullscreen Mar 9, 2021
@gkatsev gkatsev merged commit 239c9a1 into videojs:main Mar 23, 2021
infinite-persistence referenced this pull request in lbryio/lbry-desktop Jul 12, 2021
## Root-cause
`https://github.com/videojs/video.js/pull/7098`

videojs was recently upgraded.

## Change
Override videojs default theme of hiding the time control in smaller layouts.
infinite-persistence referenced this pull request in lbryio/lbry-desktop Jul 12, 2021
## Root-cause
`https://github.com/videojs/video.js/pull/7098`

videojs was recently upgraded.

## Change
Override videojs default theme of hiding the time control in smaller layouts.
tzarebczan referenced this pull request in lbryio/lbry-desktop Jul 12, 2021
## Root-cause
`https://github.com/videojs/video.js/pull/7098`

videojs was recently upgraded.

## Change
Override videojs default theme of hiding the time control in smaller layouts.
jessopb referenced this pull request in lbryio/lbry-desktop Jul 13, 2021
## Root-cause
`https://github.com/videojs/video.js/pull/7098`

videojs was recently upgraded.

## Change
Override videojs default theme of hiding the time control in smaller layouts.
infinite-persistence referenced this pull request in lbryio/lbry-desktop Jul 13, 2021
## Root-cause
`https://github.com/videojs/video.js/pull/7098`

videojs was recently upgraded.

## Change
Override videojs default theme of hiding the time control in smaller layouts.
jessopb referenced this pull request in lbryio/lbry-desktop Jul 14, 2021
## Root-cause
`https://github.com/videojs/video.js/pull/7098`

videojs was recently upgraded.

## Change
Override videojs default theme of hiding the time control in smaller layouts.
@amtins
Copy link
Contributor

amtins commented Aug 30, 2021

@gkatsev, @misteroneill we recently updated to the latest videojs version which contains this fix. However, this fix forces us to introduce the use of !important, which means we will have to do something like @infinite-persistence did here. In our case, this is not a good solution because we provide a library in which integrators can override some rules by increasing the specificity.

I don't know if I missed anything and what the BC means, but I think the !important is not necessary, based on my tests. @ipadilla4, maybe a reduced test case will help to understand why the !important is so important.

@gkatsev
Copy link
Member

gkatsev commented Aug 30, 2021

You're definitely right. We should look into and remove !important

@gkatsev
Copy link
Member

gkatsev commented Sep 28, 2021

@amtis this is being removed by #7449 and we'll get a patch released soon!

@gkatsev
Copy link
Member

gkatsev commented Oct 1, 2021

@amtins the change is in Video.js 7.15.7

edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
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.

4 participants