-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Style: update height of speakerdeck tag to be avoid scaling #5818
Style: update height of speakerdeck tag to be avoid scaling #5818
Conversation
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.
Looks fine @deepu105, thank you!
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.
The problem is actually couple lines above, not here. Check out line #5 - it does some weird magic around calculating padding-bottom. Assuming all speakerdeck presentations have 16:9 ratio, this value should be set to 56.25%
. More about the solution itself can be find here: https://benmarshall.me/responsive-iframes/
If you set it like that, all should be good without making changes to the height itself. That change is actually not ideal - it does indeed make it look okey-ish but it will break in certain scenarios.
Also, while on it, maybe we could get rid of that thingy in next line: margin-bottom:1em auto 1.3em;
since it's just wrong syntax (so it's basically it's being ignored by browsers).
@ludwiczakpawel thanks, you are right. I indeed noticed the padding but wasn't sure why the magic number was there hence I left it alone and altered the height. I fixed as per your suggestion |
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.
LGTM!
Fix #5682
What type of PR is this? (check all applicable)
Description
The speaker deck tag rendered cuts off horizontal edges due to setting 100% height. I had to remove 63px in order to render slide completely and seems to be fine in all screen sizes, including mobile
Didn't find any tests for this part to update, let me know if I missed it
Related Tickets & Documents
#5682
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Added to documentation?