Convert seo_author_* logic to a drop #184
Conversation
@benbalter This looks great, I dig the move to Ruby. I did a manual test on a blog post of mine and I notice minor differences between 2.2.0 and this PR (after beautifying the content in Atom)
If I copy the JSON-LD output to https://search.google.com/structured-data/testing-tool I got fewer errors than before. (errors with So apart from email that went missing - but is it useful? - this looks rather pretty good to me. 👏 👏 👏 |
|
@DirtyF Nice. It looks like what should be the name string is being returned as the entire author hash in the new version? How are you supplying the author? Looking at the code and tests I'm not sure how to recreate that. Unless I'm reading the diff backwards, in which case, shouldn't the Added |
|
Also to note, testing with my own site, I'm consistently seeing a 30%-40% performance improvement over |
Sorry for the confusion, to be clearer the new version returns correct values: "name":"Frank Taillandier",
"author":{"@type":"Person","name":"Frank Taillandier"}
I've got only a dozen posts on this blog, but I see a gain of 10% 😄 |
What is it supposed to return? |
spec/jekyll_seo_tag/json_ld_spec.rb
Outdated
| let(:metadata) do | ||
| { | ||
| "title" => "title", | ||
| "author" => "author", |
There was a problem hiding this comment.
We should add a test for author provided as a Hash in metadata
|
@benbalter Everything is fine. https://search.google.com/structured-data/testing-tool displays one warning left related to image in |
| require "jekyll-seo-tag/version" | ||
|
|
||
| module Jekyll | ||
| class SeoTag < Liquid::Tag | ||
| autoload :JSONLD, "jekyll-seo-tag/json_ld" | ||
| autoload :Drop, "jekyll-seo-tag/drop" | ||
| autoload :Filters, "jekyll-seo-tag/filters" |
There was a problem hiding this comment.
I recently ran into an issue with this because I didn't add lib to the $LOAD_PATH, so local tests didn't work.
lib/jekyll-seo-tag/drop.rb
Outdated
| # Should the `<title>` tag be generated for this page? | ||
| def title? | ||
| return false unless title | ||
| @text !~ %r!title=false!i |
There was a problem hiding this comment.
This would probably be good to memoize, right?
def title?
return false unless title
return @is_title if defined?(@is_title)
@is_title = @test !~ %r!title=false!i
end?
| # Page title with site title or description appended | ||
| def title | ||
| @title ||= begin | ||
| if page["title"] && site_title |
There was a problem hiding this comment.
With jekyll-titles-from-headings have populated this by the time this code is called? If so, then 😍!!!
There was a problem hiding this comment.
97% sure? A generator should be be called the render phase.
lib/jekyll-seo-tag/drop.rb
Outdated
| def name | ||
| @name ||= begin | ||
| return seo_name if seo_name | ||
| return unless homepage_or_about? |
There was a problem hiding this comment.
Note that none of these nil values will be memoized.
lib/jekyll-seo-tag/drop.rb
Outdated
| end | ||
|
|
||
| def description | ||
| @description ||= begin |
There was a problem hiding this comment.
Is the begin here necessary? I believe you can keep this on 2 lines if needed without the begin...end
lib/jekyll-seo-tag/drop.rb
Outdated
| end | ||
|
|
||
| def homepage_or_about? | ||
| ["/", "/index.html", "/about/"].include? page["url"] |
There was a problem hiding this comment.
are about.html, /index.htm, /index/ also valid?
There was a problem hiding this comment.
Moved to regex and added tests via cfe7e56.
I'm not super excited to support /index/ or about.html. (GitHub Pages lazy indexes and ugly permalinks). I could see a case for the second if there was strong desire.
lib/jekyll-seo-tag/drop.rb
Outdated
| attr_reader :context | ||
|
|
||
| def fallback_data | ||
| {} |
lib/jekyll-seo-tag/drop.rb
Outdated
| end | ||
|
|
||
| def absolute_url?(string) | ||
| string.include? "://" |
There was a problem hiding this comment.
There was a problem hiding this comment.
Ahh, good call. 8cb5953.
lib/jekyll-seo-tag/drop.rb
Outdated
| def format_string(string) | ||
| FORMAT_STRING_METHODS.each do |method| | ||
| string = filters.public_send(method, string) | ||
| end |
There was a problem hiding this comment.
This might be a good use-case for reduce, or each_with_object?
This PR moves the
seo_author_*logic in the template to Ruby, where it can be implemented as a drop. The goal is readability, testability, and performance.The resulting output should be identical, save the absence of whitespace in the JSON-LD output.
I believe the original tests should pass with the small whitespace change.
One thing to call out, I'm not 100% sold on the JSON-LD approach, but was trying to find the most efficient way to generate the hash and DRY things up.
Thoughts?
Fixes #176.