-
Notifications
You must be signed in to change notification settings - Fork 30
Support full-width looping videos in articles #15056
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
Conversation
30ac5ce to
2db2cd5
Compare
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
|
🎁🎄 Yay! Just a useless naming thought: even though fronts were where the player was born, they are a special case (letter-boxed?), while the behaviour in articles after your PR is what’s normal and expected. Once we finally have different weightings (roles) there, it may not exactly be “full-width” either. If it’s easier as is, I haven’t wrote this comment. Merry Xmas! |
Thanks @paperboyo! I like your suggestion and I've made the change here: 0f2b3b0
|
| } | ||
| subtitleSize={subtitleSize} | ||
| enableHls={enableHls} | ||
| letterboxed={true} |
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 don't know why we're touching the Fronts card here? Is calculating the aspect ratio from the video itself not enough?
Another thought - what happened with the rumours that people were looking to add an aspect ratio attribute to media atoms?
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.
Fair question, I've bundled together 2 distinct changes in this PR which is causing confusion. It probably should have been 2 PRs.
The first change uses the aspect ratio of the video to calculate the size of the video container for the purposes of reducing CLS. It has no impact on whether or not the video displays letterboxes. It just follows on from this PR where it was missed: #15029
The second change introduces the concept of letterboxes which can be enabled or disabled. Since the default state is to disable letterboxes (see Mat's comment), we need to change the Fronts card to enable them again.
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.
Another thought - what happened with the rumours that people were looking to add an aspect ratio attribute to media atoms?
If I understand you correctly, such a field exists now. But it's a string (e.g. "5:4") and we need a numerical value so I opted to use the dimensions instead.
RikRootsGuardian
left a 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.
Happy to approve, following discussion.
0f2b3b0 to
dd67624
Compare
What does this change?
Does two things:
containerAspectRatio. This follows on from Set aspect ratio of containers to prevent CLS #15029Why?
Editorial have requested it. There is a precedent because this is how images currently behave in articles.
Screenshots