Include jekyll-seo-tag when present #109
Conversation
This checks for the inclusion of `jekyll-seo-tag` via site.gems and includes it in head.html when available making integration with the jekyll-seo-tag plugin seamless.
| @@ -13,4 +13,8 @@ | |||
| {% if jekyll.environment == 'production' and site.google_analytics %} | |||
| {% include google-analytics.html %} | |||
| {% endif %} | |||
|
|
|||
| {% if site.gems contains 'jekyll-seo-tag' %} | |||
| {% include seo.html %} | |||
There was a problem hiding this comment.
why not just use the default settings instead of enforcing an advanced usage?
{% if site.gems contains 'jekyll-seo-tag' %}
{% seo %}
{% endif %}
There was a problem hiding this comment.
That won't work because liquid doesn't lazy evaluate, the include is to lazily evaluate only, and only if the tag is present.
Otherwise, you will get an undefined tag error.
There was a problem hiding this comment.
That won't work because liquid doesn't lazy evaluate
OK. Agreed.
But that doesn't explain the need of having {% seo title=false %} by default.
Also, this solution wont work in the edge-situation where the user has the plugin loaded only via the :jekyll_plugins group in Gemfile
There was a problem hiding this comment.
Also, this solution wont work in the edge-situation where the user has the plugin loaded only via the
:jekyll_pluginsgroup inGemfile
I don't know that I'd call that an edge case, as it is one of three official ways to install a plugin, as documented on jekyllrb.com
There was a problem hiding this comment.
edge case because, the plugin's README has instructed the user to include the plugin under gems:
There was a problem hiding this comment.
Documentation of this particular plugin says to install via site.gems, so I'd say the how to install a plugin documentation is irrelevant as here, yeah you can have plugins in _plugins but that doesn't apply to the gem jekyll-seo-tag, it's own installation instructions should take precedence here.
There was a problem hiding this comment.
But that doesn't explain the need of having {% seo title=false %} by default.
@ashmaroli no particular reason, except that it was the minimal implementation since title is already being defined earlier in head.html and redefining the <title> tag would be stepping on toes.
|
I like the intent here, but the implementation feels hacky and overly complex. Yes, you get metadata in the head, but at the expense of weird liquid and an extra include. I'd rather fix this upstream first in core with some sort of "call this tag if it exists" wrapper, if possible, rather than make the default solution a one-line include (although that's pretty creative). |
|
@benbalter agree that it's hacky, I was actually suprised that Liquid does not allow lazy evaluation (e.g Not sure if we should try to deal with this in Jekyll core, or bring it all the way up to liquid since it's kinda a language issue. |
|
Since #139 |
This checks for the inclusion of
jekyll-seo-tagvia site.gems andincludes it in head.html when available making integration with the
jekyll-seo-tag plugin seamless.