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

Added `video_embed` tag that accepts kwargs #20

Merged
merged 1 commit into from May 2, 2014

Conversation

Projects
None yet
2 participants
@bburan
Contributor

bburan commented Mar 24, 2014

Some video backends support optional arguments embedded in the URL query
string that adjust how the video is displayed (e.g. Youtube supports
rel=0 which means don't show related videos). Created a tag that
takes keyword arguments and embeds them into the URL query string.

This tag also supports appending to existing queries hard-coded in the
video backend URL (e.g. Youtube's backend has a hard-coded query of
wmod=opaque).

@@ -2,6 +2,7 @@
import requests
import json

import urllib

This comment has been minimized.

@yetty

yetty Mar 24, 2014

Collaborator

Remove blank line above and add one blank line below (just code style standard)

return self.pattern_url.format(code=self.code, protocol=self.protocol)
url = self.pattern_url.format(code=self.code, protocol=self.protocol)
if self._query is not None:
result = list(urlparse.urlparse(url))

This comment has been minimized.

@yetty

yetty Mar 24, 2014

Collaborator

This and following four lines are really complicated and not easy to understand at all. Are you sure it cannot be done simpler? Take a look at requests library. It could be what you looking for.

Anyway it has to be covered with tests.

@@ -169,6 +178,8 @@ def get_embed_code(self, width, height):
"""
Returns embed code rendered from template :py:data:`template_name`.
"""
# Need to cache this locally so that it can be accessed by the

This comment has been minimized.

@yetty

yetty Mar 24, 2014

Collaborator

I am not really sure, what do you mean. Can you explain?



@register.simple_tag
def video_embed(url, size='small', **kwargs):

This comment has been minimized.

@yetty

yetty Mar 24, 2014

Collaborator

Why do you need new tag? Try to inherit new functionality to VideoNode.

@yetty

This comment has been minimized.

Collaborator

yetty commented Mar 24, 2014

Hi,
thank you for your pull request. I have added few comments direct to relevant lines.

  • please write test to cover your new functionality. From tests it is obvious, how does it really works (and if it works at all).
  • edit package documentation and explain usage of your functionality.

@yetty yetty added this to the v0.9 milestone Mar 24, 2014

@yetty yetty added the enhancement label Mar 24, 2014

@@ -175,7 +175,7 @@ def __repr__(self):


@register.filter(is_safe=True)
def embed(backend, size='small'):
def embed(backend, size='small', **kwargs):

This comment has been minimized.

@yetty

yetty Mar 24, 2014

Collaborator

**kwargs added, but not used anywhere?

@bburan

This comment has been minimized.

Contributor

bburan commented Mar 24, 2014

No problem. I will address your comments and update. It may take a while as I will need to figure out how the VideoNode works and how one would extract kwargs from the template tag as well as handle both behaviors (e.g. block and inline tags) as well as the context manager (i.e. the 'as' statement).

@yetty

This comment has been minimized.

Collaborator

yetty commented Mar 24, 2014

@yetty yetty modified the milestones: v0.10, v0.9 Apr 4, 2014

Updated video template tag to accept kwargs
Some video backends support optional arguments embedded in the URL query
string that adjust how the video is displayed (e.g. Youtube supports
rel=0 which means don't show related videos).  Updated the template tag
to accept optional KEY=VALUE pairs that will be added onto the embedded
URL as a query string.  Also added support for specifying default query
strings on a per-backend basis (e.g. so that all YouTube urls have
?rel=0 added to the end).

Added appropriate tests and documentation.
@bburan

This comment has been minimized.

Contributor

bburan commented May 1, 2014

@yetty - Finally got around to implementing the changes you suggested. I amended the original commit and force-pushed to github so that all changes would appear as a single commit if accepted. I also added unit tests and documentation for the new features. Please let me know what you think and I can make changes as needed.

yetty added a commit that referenced this pull request May 2, 2014

Merge pull request #20 from bburan/bburan/embed-with-kwargs
Added `video_embed` tag that accepts kwargs

@yetty yetty merged commit 5857d80 into jazzband:master May 2, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@yetty

This comment has been minimized.

Collaborator

yetty commented May 2, 2014

Great job done. Merged.

@yetty yetty referenced this pull request Jul 26, 2014

Merged

Options in embed tag #36

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