[SVCS-671] Fix scrollbars on video #318
Conversation
1 similar comment
There was a problem hiding this comment.
LGTM and works locally on Chrome, Firefox, Safari and IE 11. Such a simple but elegant change that fixes the super-annoying scroll bar flickering issue. 👍 🎆
A few non-blocking issues:
- Add a
makocomment - Add a test that make sure that video files are rendered with this
<style>in the HTML header in the iframe. Create or find existing ticket for us to better keep track of this issueI couldn't read the description of the PR.
Back to you @birdbrained , thanks!
| <script> | ||
| window.pymChild.sendMessage('embed', 'embed-responsive-16by9'); | ||
| </script> | ||
| </script> |
There was a problem hiding this comment.
👍 for an empty line for GitHub
There was a problem hiding this comment.
It wasn't me it was my vim settings.
| @@ -1,3 +1,10 @@ | |||
| <style> | |||
There was a problem hiding this comment.
-
Can you add a
makocomment##explaining why this works? If it is purely magic, then what it does/fixes is fine. In this way, others can know why this is a must-have piece without taking a look at the commit message. -
I had worried that
<style>may not work inbodybut local testing confirmed that MFR put it in the<header>tag. 😌
There was a problem hiding this comment.
Its not magic - its just default browser styles add margin and padding on the body tag. This is something very common and most css libraries like bootstrap or reset.css do this. - I thought this was common knowledge but I can add a comment if we need it. The only reason this is in a style tag and not in a CSS file is because this way we don't need to worry about it affecting other extensions by accident - this will only affect videos. Lastly yep style tags will work anywhere. html5 spec actually doesn't even require head and body tags to exist - the browser automatically inserts them if they're not there.
There was a problem hiding this comment.
- I see. Thanks for the clarification. I didn't understand why this solve the scroll bar flickering issue and was asking that a comment is helpful here. I tried to use this solution to fix similar scroll bar flickering issue on chrome for images but it didn't work.
- We can not rely on HTML5 since there is IE11. This is where my initial worry came from until I successfully tested this in IE11.
There was a problem hiding this comment.
@birdbrained Thanks for the explanation. No changes required. I will add a brief comment and a quick test during merge. Move to RTM 🎆.
| @@ -1,3 +1,10 @@ | |||
| <style> | |||
There was a problem hiding this comment.
- I see. Thanks for the clarification. I didn't understand why this solve the scroll bar flickering issue and was asking that a comment is helpful here. I tried to use this solution to fix similar scroll bar flickering issue on chrome for images but it didn't work.
- We can not rely on HTML5 since there is IE11. This is where my initial worry came from until I successfully tested this in IE11.
Ticket
https://openscience.atlassian.net/browse/SVCS-671
Purpose
This makes it so videos rendered by the MFR will not display with scrollbars inside the OSF.
Changes
Removes any space between the edges of the iframes interior and the video.
Side effects
doubtful, though it was mentioned sometimes wierd stuff happens in IE.
QA Notes
Deployment Notes