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

Support video file previews #1817

Merged
merged 1 commit into from Dec 14, 2017

Conversation

Projects
None yet
4 participants
@MaxLeiter
Member

MaxLeiter commented Dec 9, 2017

will rebase on #1806 if it's merged in first (assume it will be)

screen shot 2017-12-09 at 3 25 59 pm

screen shot 2017-12-09 at 3 25 52 pm

@xPaw xPaw added the Type: Feature label Dec 9, 2017

{{#if thumb}}
<a class="toggle-thumbnail" href="{{link}}" target="_blank" rel="noopener">
<img src="{{thumb}}" class="thumb">
{{#equal type "video"}}

This comment has been minimized.

@McInkay

McInkay Dec 11, 2017

Member

This file should probably start getting abstracted out if we are going to add a bunch of other ones to it. I know you are only adding 2 at this point, but it's going to get too big, I think.

This comment has been minimized.

@MaxLeiter

MaxLeiter Dec 13, 2017

Member

Agreed. need to investigate handlebars {{else if}}, but I can't figure out how it works with helpers. For a future PR.

This comment has been minimized.

@McInkay

McInkay Dec 13, 2017

Member

Yeah, I wasn't meaning for this PR, it's just something we should do before we start adding third party site previews and stuff. The 2 PRs you have up are fine, it's not too big yet, it's just clearly going to get too big.

if (!preview.link.startsWith("https://")) {
break;
}
preview.res = res.type;

This comment has been minimized.

@McInkay

McInkay Dec 11, 2017

Member

Hmmm, here's a question, why do we have 2 different types that we pass to the client?

This comment has been minimized.

@xPaw

xPaw Dec 11, 2017

Member

One is "video" to know which template to render, one is full content-type like "video/mp4".

This comment has been minimized.

@McInkay

McInkay Dec 11, 2017

Member

Ah right, so it's essentially 2 different levels of type of preview. 1 to decide which preview template to use, and another for the preview template (in this case, it's just passed to the video element, but the template could theoretically do something else with it). Makes sense.

@astorije

This comment has been minimized.

Member

astorije commented Dec 13, 2017

👏

@McInkay

This comment has been minimized.

Member

McInkay commented Dec 13, 2017

Hmm, any reason you didn't merge, @astorije ? We have 2 approvals now.

Sadly, I can't approve for some reason as I get this, so someone else will have to.

@astorije

This comment has been minimized.

Member

astorije commented Dec 13, 2017

any reason you didn't merge, @astorije ?

I just thought this PR required changes per @MaxLeiter's PR description, and also I didn't test it. Feel free to merge whenever you deem it ready. Great job @MaxLeiter!

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Dec 14, 2017

Probably should be re-approved, changed layout of msg_preview.tpl quite a bit.

ping @astorije @YaManicKill

@McInkay

This comment has been minimized.

Member

McInkay commented Dec 14, 2017

Looks good, merging :-) thanks @MaxLeiter

@McInkay McInkay merged commit 8b52825 into thelounge:master Dec 14, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MaxLeiter MaxLeiter deleted the MaxLeiter:videos branch Dec 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment