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

up the minimum size of the public share video a bit, use 16/9 ratio #625

Merged
merged 1 commit into from
Jul 29, 2016

Conversation

jancborchardt
Copy link
Member

fix #603 – ended up using half the size of Youtube’s 854*480px since that is a bit too big.

Please review @icewind1991 @oparoz @schiessle

@jancborchardt jancborchardt added enhancement design Design, UI, UX, etc. 3. to review Waiting for reviews feature: sharing labels Jul 28, 2016
@jancborchardt jancborchardt added this to the Nextcloud 11.0 milestone Jul 28, 2016
@mention-bot
Copy link

@jancborchardt, thanks for your PR! By analyzing the annotation information on this pull request, we identified @LukasReschke, @MorrisJobke and @oparoz to be potential reviewers

@oparoz
Copy link
Member

oparoz commented Jul 28, 2016

@jancborchardt Does it work nicely on mobile?

@jancborchardt
Copy link
Member Author

jancborchardt commented Jul 28, 2016

@oparoz yep it does – you can simply make your window smaller to test. :) That’s also why I chose the smaller 427 * 240px instead of the full 854 * 480px. It will always use maximally 100% of the viewport width.

@icewind1991
Copy link
Member

Shouldn't this be min-width/height?

@jancborchardt
Copy link
Member Author

@icewind1991 no, because then on mobile when the screen is smaller than this it will cause scrolling.

@jancborchardt
Copy link
Member Author

Sooo – @oparoz @icewind1991 can you actually test this and give it thumbs up? ;D

@icewind1991
Copy link
Member

@icewind1991 no, because then on mobile when the screen is smaller than this it will cause scrolling.

Also set max-width 100%

Imo this is to small for non mobile screens

@MorrisJobke
Copy link
Member

Imo this is to small for non mobile screens

Indeed:

bildschirmfoto 2016-07-28 um 17 50 45

Doubled size:

bildschirmfoto 2016-07-28 um 17 51 16

@jancborchardt jancborchardt force-pushed the video-container-size branch from 8a305c4 to e92723d Compare July 29, 2016 08:45
@jancborchardt jancborchardt force-pushed the video-container-size branch from e92723d to 748745d Compare July 29, 2016 08:47
@jancborchardt
Copy link
Member Author

Seems there was some inline stuff setting other max-widths which messed it up. Needed to add !important to fix it. Please review again :)

@MorrisJobke
Copy link
Member

A lot better now 👍 Works

@icewind1991
Copy link
Member

👍

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 29, 2016
@jancborchardt jancborchardt merged commit 696ff90 into master Jul 29, 2016
@jancborchardt jancborchardt deleted the video-container-size branch July 29, 2016 13:36
@MorrisJobke
Copy link
Member

@karlitschek Port to stable10?

@jancborchardt
Copy link
Member Author

I’d say it’s a nice detail/fix for sharing and a backport would be cool.

@karlitschek
Copy link
Member

nice. please backport 👍

@jancborchardt
Copy link
Member Author

Backport at #668

GitHubUser4234 pushed a commit to GitHubUser4234/server that referenced this pull request Aug 30, 2016
up the minimum size of the public share video a bit, use 16/9 ratio
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish design Design, UI, UX, etc. enhancement feature: sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Video sizing on the public share page
6 participants