-
Notifications
You must be signed in to change notification settings - Fork 297
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
Convert author, image, and JSON-LD to dedicated drops #229
Conversation
…oid a breaking change
alias_method :to_s, :name | ||
|
||
def twitter | ||
return @twitter if defined? @twitter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@twitter
made me look twice. When Ruby and Twitter semantics collide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/jekyll-seo-tag/json_ld.rb
Outdated
@@ -18,62 +17,6 @@ module JSONLD | |||
:links => "sameAs", | |||
:canonical_url => "url", | |||
}.freeze | |||
|
|||
def json_ld |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the goal is backwards compatibility, do we need to keep this method as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop
extended JsonLD
(which is where I moved this method to). Theoretically, if someone extended the module into another class, this would be a breaking change. I'll can duplicate it here with a note that it should be removed at the next major release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a smart move, really appreciate the effort you make to share your vision @benbalter.
The code is more modular and it's great if this helps provide more extensibility to the developers.
Didn't know much about Drops before, but starting to get the big picture.
Best branch name ever on 🔝 of that.
Learning the power of drops myself. They're intended to be lazy loaders, but essentially act as super smart hashes. The TL;DR is that any public method is exposed to liquid as both |
I began this PR by trying to simplify the
author
,author_string_or_hash
, andsite_author_hash
methods by moving the author logic to its own dedicated drop with smaller methods.Having done that, I saw that the
image
could be similarly pushed into its own drop and that or existing JSON-LD implementation, was essentially a drop (although we were including it in the main class, rather than subclassing it).In the end, this PR (or potentially three PRs, I made the commits distinct) should be fully backwards compatible, but should move the
author
,image
, andjson_ld
logic out of the already-very-long Jekyll::SeoTag::Drop and into their own drop-based classes.A few things to call out:
I wrote the author drop, such that, it could be pulled out into its own library, and theoretically, be a drop-in replacement for
page.author
, in thatPageDrop#to_s
will return the author name, as if it were a string (e.g. via liquid), and it can also be accessed as a hash to retrieve additional metadata, when present.The tests pass almost entirely unchanged. All I did was move the tests from
drop_spec
tofoo_drop_spec
and update the method calls form e.g.,subject.author
to justauthor
.The integration spec was unchanged, save the order of the elements in the JSON output.
In the long run, I'd ideally like Jekyll SEO to expose really good drops to the HTML template, and allow users to override the template with their own if they want logic/optimization beyond the majority use case (but have the tools to do so without starting from scratch).
Thoughts? Is this easier to maintain? Harder? Too much for one PR? Won't be offended either way, but wanted to push up the idea.