-
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
fix: Audio only mode styling conflicts with fluid mode #7724
fix: Audio only mode styling conflicts with fluid mode #7724
Conversation
Codecov Report
@@ Coverage Diff @@
## main #7724 +/- ##
=======================================
Coverage 80.89% 80.89%
=======================================
Files 116 116
Lines 7455 7455
Branches 1806 1806
=======================================
Hits 6031 6031
Misses 1424 1424
Continue to review full report at Codecov.
|
@include apply-aspect-ratio(1, 1); | ||
} | ||
|
||
.video-js.vjs-fill { | ||
.video-js.vjs-fill:not(.vjs-audio-only-mode) { |
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.
Is there a reason why we wouldn't be able to add a rule with a target like .video-js.vjs-audio-only-mode
after these rules? That would be of equal hierarchy as the .video-js.<ratio-class-name>
targets and should override them just because it's further down in the stylesheet. That way you wouldn't need to specify :not()
.
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.
And same thought for the targeting on line 82. It might just be easier a bit more terse to have a small .video-js.vjs-audio-only-mode
rule after the ones modified here. That way it's more declarative and less conditional.
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.
Summary following offline discussion:
This is a good suggestion and it would probably be ideal to do it this way, but overriding the player's height
via CSS in these places disables the player.height()
setter method, on which audioOnlyMode()
currently depends. So I think we should leave this as is for now, and if we decide to rework audioOnlyMode()
later not to use player.height()
we can do that, but it's out of scope for this PR.
550a895
to
430fe4b
Compare
* css changes * make audio only mode player responsive in fluid mode
Description
Disable all fluid and fill mode CSS when in audio only mode, except for that which adjusts the player's width responsively. This fixes a styling conflict that broke the player UI when both fluid and audio only mode were simultaneously enabled.