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

Fixes brave/brave-browser#1229 Webtorrent media scaling. #2261

Closed
wants to merge 1 commit into from
Closed

Fixes brave/brave-browser#1229 Webtorrent media scaling. #2261

wants to merge 1 commit into from

Conversation

kylemiller3
Copy link
Contributor

@kylemiller3 kylemiller3 commented Apr 18, 2019

Screenshots of changes:
https://i.imgur.com/Z58xjWr.png
https://i.imgur.com/NRpTmFO.png

Submitter Checklist:

Test Plan:

Load a video torrent in webtorrent and see it it fills the body of the window in letterbox effect.
Load an image torrent in webtorrent and see the iframe fills the body of the window.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@bbondy
Copy link
Member

bbondy commented Jul 17, 2019

Is this still valid? CC @feross

@kylemiller3
Copy link
Contributor Author

Should still work. Pretty trivial fixes

style.marginTop = '0px'
style.marginLeft = '0px'
style.marginBottom = '0px'
style.marginRight = '0px'
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 these styles cannot be applied using CSS?

video {
  padding: 0;
  margin: 0;
}

Copy link
Contributor Author

@kylemiller3 kylemiller3 Jul 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest it's been so long ago now I don't quite remember if there is a reason or it was just something I overlooked. I believe I did try to use straight up CSS to make it work but I think it needs the media to load first? Something to that effect.

feross added a commit that referenced this pull request Jul 25, 2019
@feross
Copy link
Contributor

feross commented Jul 25, 2019

Thanks for the PR! I needed to add support for <audio> tags and make some other changes so it was easier to make a new PR: #3005

@feross feross closed this Jul 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants