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

fix: Audio only mode styling conflicts with fluid mode #7724

Merged

Conversation

alex-barstow
Copy link
Contributor

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.

@codecov
Copy link

codecov bot commented Apr 13, 2022

Codecov Report

Merging #7724 (550a895) into main (c5a0376) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 550a895 differs from pull request most recent head 430fe4b. Consider uploading reports for the commit 430fe4b to get more accurate results

@@           Coverage Diff           @@
##             main    #7724   +/-   ##
=======================================
  Coverage   80.89%   80.89%           
=======================================
  Files         116      116           
  Lines        7455     7455           
  Branches     1806     1806           
=======================================
  Hits         6031     6031           
  Misses       1424     1424           
Impacted Files Coverage Δ
src/js/video.js 93.75% <ø> (ø)
src/js/player.js 88.37% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5a0376...430fe4b. Read the comment docs.

@include apply-aspect-ratio(1, 1);
}

.video-js.vjs-fill {
.video-js.vjs-fill:not(.vjs-audio-only-mode) {
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@alex-barstow alex-barstow force-pushed the fix/audio-only-mode-style-conflicts branch from 550a895 to 430fe4b Compare April 14, 2022 20:50
@alex-barstow alex-barstow merged commit 145aba6 into videojs:main Apr 15, 2022
@alex-barstow alex-barstow deleted the fix/audio-only-mode-style-conflicts branch April 15, 2022 16:55
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
* css changes

* make audio only mode player responsive in fluid mode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants