Conversation
| tags['og:locale'] = self.language | ||
| tags['og:site_name'] = self.site_name | ||
|
|
||
| for _, adapter in getAdapters((self.context,), IAdditionalMetatags): |
There was a problem hiding this comment.
I know that passing a tuple is more efficient, but is also less clear; can we change this for:
for _, adapter in getAdapters([self.context], IAdditionalMetatags):|
|
||
|
|
||
| class IAdditionalMetatags(Interface): | ||
| """ Interface to provide additional metatags to those provided by default """ |
There was a problem hiding this comment.
docstring should not include no trailing space and must end with a period "."
There was a problem hiding this comment.
Would adding flake8-extensions = flake8-docstrings to buildout.cfg avoid this manual review, @hvelarde? About trailing space and the period?
There was a problem hiding this comment.
I think I removed it at some point because it was causing more issues than the ones solving; we can try it in a different PR.
| """ Interface to provide additional metatags to those provided by default """ | ||
|
|
||
| def metatags(): | ||
| """ a dict with additional metatags """ |
| provideAdapter(NewsItemMetatagsAdapter) | ||
| viewlet = self.viewlet(self.obj) | ||
| html = viewlet.render() | ||
| self.assertIn('one_key', html) |
There was a problem hiding this comment.
I would love to make this tests a little bit more resilient using lxml instead, but it's up to you.
| ) | ||
|
|
||
|
|
||
| class IAdditionalMetatags(Interface): |
There was a problem hiding this comment.
I strongly dislike this name, what do you think about IOGProperties or something like that?
There was a problem hiding this comment.
I used a generic name because in some cases one may want to add some non-og metatags. I don't know whether now there is any disalignment between OpenGraph and Twitter Card metatags, but at some point there was the need to add some tags that were not specifically OpenGraph.
But anyway, it's just a naming issue, so I can rename it without any problem.
idgserpro
left a comment
idgserpro
left a comment
There was a problem hiding this comment.
LGTM. Don't know if my suggestion in https://github.com/collective/sc.social.like/pull/147/files#r143023636 would avoid docstring problems, let's see if @hvelarde has an opinion about that.
rodfersou
left a comment
rodfersou
left a comment
There was a problem hiding this comment.
Facebook and Twitter has some different meta tags https://github.com/collective/sc.social.like/blob/master/sc/social/like/plugins/facebook/templates/metadata.pt https://github.com/collective/sc.social.like/blob/master/sc/social/like/plugins/twitter/templates/metadata.pt and twitter uses the "name" instead of "property" in it's tags.
is it possible to use the same mechanism for these tags in these plugins?
There was a problem hiding this comment.
I renamed again the interface and made some changes to the documentation.
in the process of reviewing the code I discovered a problem: OG tags can have multiple values as defined in the Array section of the documentation; that means we can't use a dictionary for describing the metadata or we'll be unable of implementing things like this:
<meta property="og:article:tag" content="foo" />
<meta property="og:article:tag" content="bar" />we need to think a little bit more before merging this.
|
@rodfersou Twitter meta tags use the standard BTW, @erral I would like to find a way to be able to dynamically add tabs to the control panel configlet (for third-party plugins, for instance); do you have any suggestion? |
@hvelarde we can show or hide the tabs dynamically with some lines of javascript code. |
|
Would it be acceptable to allow lists to be values of the dict and then when rendering the template check the type of the value and if it is a list, render it properly? |
|
@erral sound doable, but we will need to make the documentation a little bit more explicit then. @rodfersou I think that's not needed right now; what we need is a way to add dynamically tabs to the configlet. |
collective.plonetruegallery has a similar usecase: plugins can have configuration and if enabled it is shown in the control panel config. |
|
@erral IMO that's way to complicated; we only need to do the same thing is done in behaviors to extend the configuration form. |
4 participants
I needed a way to extend the default metadata provided by the product, and implemented it using adapters.
To extend the default metadata, you need to provide an adapter for the given content type and return a dict with the extended metadata.
You can also override preexisting metadata (if you want to change for instance og:title or any other.
This idea of using an adapter comes from collective.opengraph a preexisting product that provides just opengraph metadata support.
Any comments are welcomed, of course!