Skip to content
This repository has been archived by the owner on May 11, 2020. It is now read-only.

Talk videos #72

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Talk videos #72

wants to merge 8 commits into from

Conversation

leesto
Copy link
Member

@leesto leesto commented Mar 30, 2018

Add support to the talks page for showing:

  • An embedded YouTube video
  • An embed of slides
  • A link to joind.in

We add this data to some talk files to be able to test this

tags:
- {name: "TwigExtension"}

app.websiteGenerator.twigExtension.slidesEmber:
Copy link
Contributor

Choose a reason for hiding this comment

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

Guessing this should be slidesEmbed?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup - fixed

@@ -0,0 +1,5 @@
{% if talk.joindinUrl %}
<div style="text-align:right;">
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a text-right class for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was having issues yesterday with some of the inbuilt class for tailwindcss not showing up or working (I had the same issue with h-full). Seems to not be a problem now though, so I must have done something!

$iFrameUrl = "https://mozilla.github.io/pdf.js/web/viewer.html?file={$talk->getSlidesUrl()}#zoom=page-fit";
} elseif (strpos($talk->getSlidesUrl(), 'slideshare')) {
// We use embedly to embed slideshare
$embedString = '<blockquote class="embedly-card"><h4><a href="'.$talk->getSlidesUrl().'">'.$talk->getTitle().'</a></h4></blockquote><script async src="//cdn.embedly.com/widgets/platform.js" charset="UTF-8"></script>';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want markup like this within PHP code? Or should it be split into different Twig partials or something else? Just seems a little weird.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I've only done 3 so far, but we could end up with about 7 or so different embed/sharing approaches which all subtly different html to render.
I wasn't completely convinced about putting the logic to determine which one we should use within the templates, but I'm open for suggestions.

$actual = $slidesEmbedExtension->getSlidesLink($talk);
$this->assertEquals(
'<a href="https://talks.philsturgeon.uk/instances/phpsw-jan18/http-caching-to-save-the-polar-bears/html/#/" class="block mb-1" target="_blank">
<svg class="fill-current w-4 mr-1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 20 20"><path d="M9.26 13a2 2 0 0 1 .01-2.01A3 3 0 0 0 9 5H5a3 3 0 0 0 0 6h.08a6.06 6.06 0 0 0 0 2H5A5 5 0 0 1 5 3h4a5 5 0 0 1 .26 10zm1.48-6a2 2 0 0 1-.01 2.01A3 3 0 0 0 11 15h4a3 3 0 0 0 0-6h-.08a6.06 6.06 0 0 0 0-2H15a5 5 0 0 1 0 10h-4a5 5 0 0 1-.26-10z"></path></svg>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to include the markup within the texts? I've usually tried to avoid this and focus on the data properties and methods, just so that the tests don't break if a class is changed in a template.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is, the main thing we're doing with this extension is generating the relevant markup to embed the slides for a given service.

We could split each slides service up into it's own class and sort of handle it that way, which might make things a bit neater

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we keep it this way for now, and then look at refactoring it into different services post-launch?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants