Adds `link` Liquid tag to make generation of URL's easier #4624

Merged
merged 4 commits into from Mar 24, 2016

Conversation

Projects
None yet
8 participants
@jeffkole
Contributor

jeffkole commented Mar 2, 2016

This tag mirrors the post_tag functionality but for collections instead of just posts.

Adds collection_tag
This tag mirrors the post_tag functionality but for collections instead of just
posts.
@jeffkole

This comment has been minimized.

Show comment
Hide comment
@jeffkole

jeffkole Mar 2, 2016

Contributor

This is a possible solution to #2252.

If the design looks good and the code is good, I can also add docs to describe how to use it.

Contributor

jeffkole commented Mar 2, 2016

This is a possible solution to #2252.

If the design looks good and the code is good, I can also add docs to describe how to use it.

@parkr parkr added the undetermined label Mar 9, 2016

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Mar 9, 2016

Member

What's the benefit to linking to a collection? I'm not seeing one at the moment since collections refer to a group of things and a post is a single entity.

Member

mattr- commented Mar 9, 2016

What's the benefit to linking to a collection? I'm not seeing one at the moment since collections refer to a group of things and a post is a single entity.

@jeffkole

This comment has been minimized.

Show comment
Hide comment
@jeffkole

jeffkole Mar 13, 2016

Contributor

Perhaps this is just misnamed. The tag links to a specific item in a collection, not to the collection itself.

If you have a collection that generates pages (ie, output: true), then you might want to link directly to those pages. For instance, if you have a collection of blog authors and a page with bio is generated for each one, you may want to link to that page for a specific author from the byline of a post page.

Contributor

jeffkole commented Mar 13, 2016

Perhaps this is just misnamed. The tag links to a specific item in a collection, not to the collection itself.

If you have a collection that generates pages (ie, output: true), then you might want to link directly to those pages. For instance, if you have a collection of blog authors and a page with bio is generated for each one, you may want to link to that page for a specific author from the byline of a post page.

@budparr

This comment has been minimized.

Show comment
Hide comment
@budparr

budparr Mar 13, 2016

Contributor

Not commenting on the PR, necessarily, but the use-case you bring up, @jeffkole, can be accomplished in 3.1 with something like this:

{% assign author = site.authors | where: 'slug', page.author %}

<a href="{{ author[0].url }}"> {{ author[0].title }}</a> 
Contributor

budparr commented Mar 13, 2016

Not commenting on the PR, necessarily, but the use-case you bring up, @jeffkole, can be accomplished in 3.1 with something like this:

{% assign author = site.authors | where: 'slug', page.author %}

<a href="{{ author[0].url }}"> {{ author[0].title }}</a> 
@jeffkole

This comment has been minimized.

Show comment
Hide comment
@jeffkole

jeffkole Mar 14, 2016

Contributor

@budparr Let's say that I have another collection that is of books I have read and written summaries of, and that I like to link to those summary pages in posts that I write. Don't you think it would be nicer for the user to write

In [The Hard Thing About Hard Things]({% collection_url books hard-thing-about-hard-things %}), Ben Horowitz describes...

than

In [The Hard Thing About Hard Things]({% assign book = site.books | where: 'slug', 'hard-thing-about-hard-things' %}{{ book[0].url }}), Ben Horowitz describes...

The first strikes me as a nice clean interface to get a specific job done. It can be documented and tested. The second is obviously more flexible, but it requires knowing more about the internals of Jekyll to use it.

Contributor

jeffkole commented Mar 14, 2016

@budparr Let's say that I have another collection that is of books I have read and written summaries of, and that I like to link to those summary pages in posts that I write. Don't you think it would be nicer for the user to write

In [The Hard Thing About Hard Things]({% collection_url books hard-thing-about-hard-things %}), Ben Horowitz describes...

than

In [The Hard Thing About Hard Things]({% assign book = site.books | where: 'slug', 'hard-thing-about-hard-things' %}{{ book[0].url }}), Ben Horowitz describes...

The first strikes me as a nice clean interface to get a specific job done. It can be documented and tested. The second is obviously more flexible, but it requires knowing more about the internals of Jekyll to use it.

@budparr

This comment has been minimized.

Show comment
Hide comment
@budparr

budparr Mar 14, 2016

Contributor

@jeffkole - Neither one seems all that easy (I think if I were doing that I'd grab the url directly).

But, I had/have no intention to argue the elegance of the solution. I only brought it up because being able to access a collection item's slug is new as of 3.1, and makes accessing an item much easier than before, so I wanted to make sure you were aware of it.

Cheers!

Contributor

budparr commented Mar 14, 2016

@jeffkole - Neither one seems all that easy (I think if I were doing that I'd grab the url directly).

But, I had/have no intention to argue the elegance of the solution. I only brought it up because being able to access a collection item's slug is new as of 3.1, and makes accessing an item much easier than before, so I wanted to make sure you were aware of it.

Cheers!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 15, 2016

Member

@jeffkole I'm interested in a multi-purpose tag which can work for more than just collection documents.

What do you think about {% link _books/hard-things-about-hard-things.md %}? It'd check against all documents in site.docs_to_write for that relative_path and output the url for that document, or error otherwise.

Member

parkr commented Mar 15, 2016

@jeffkole I'm interested in a multi-purpose tag which can work for more than just collection documents.

What do you think about {% link _books/hard-things-about-hard-things.md %}? It'd check against all documents in site.docs_to_write for that relative_path and output the url for that document, or error otherwise.

@parkr parkr added this to the undetermined milestone Mar 15, 2016

@parkr parkr added feature and removed undetermined labels Mar 15, 2016

@jeffkole

This comment has been minimized.

Show comment
Hide comment
@jeffkole

jeffkole Mar 17, 2016

Contributor

@parkr Updated to be a link tag. Let me know what you think. Probably could stand a rebase, and then I can add documentation, too.

Contributor

jeffkole commented Mar 17, 2016

@parkr Updated to be a link tag. Let me know what you think. Probably could stand a rebase, and then I can add documentation, too.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 17, 2016

Contributor

IMO, rebasing is dead now. Github just made it easy to review changes without rebasing.

Contributor

envygeeks commented Mar 17, 2016

IMO, rebasing is dead now. Github just made it easy to review changes without rebasing.

test/test_tags.rb
@@ -14,6 +14,9 @@ def create_post(content, override = {}, converter_class = Jekyll::Converters::Ma
if override['read_posts']
site.posts.docs.concat(PostReader.new(site).read_posts(''))
end
+ if override['read_collections']

This comment has been minimized.

@envygeeks

envygeeks Mar 17, 2016

Contributor

It would be preferable to make both these if's single line and stack them above the other.

@envygeeks

envygeeks Mar 17, 2016

Contributor

It would be preferable to make both these if's single line and stack them above the other.

This comment has been minimized.

@jeffkole

jeffkole Mar 17, 2016

Contributor

Like this?

site.posts.docs.concat(PostReader.new(site).read_posts('')) if override['read_posts']
CollectionReader.new(site).read if override['read_collections']
@jeffkole

jeffkole Mar 17, 2016

Contributor

Like this?

site.posts.docs.concat(PostReader.new(site).read_posts('')) if override['read_posts']
CollectionReader.new(site).read if override['read_collections']

This comment has been minimized.

@envygeeks

envygeeks Mar 17, 2016

Contributor

Yes, that looks much cleaner!

@envygeeks

envygeeks Mar 17, 2016

Contributor

Yes, that looks much cleaner!

This comment has been minimized.

@parkr

parkr Mar 19, 2016

Member

Isn't that over 90 chars?

@parkr

parkr Mar 19, 2016

Member

Isn't that over 90 chars?

This comment has been minimized.

@envygeeks

envygeeks Mar 19, 2016

Contributor

@parkr the bot (and I believe Rubocop too) doesn't track line width on Jekyll/Jekyll for now because we have a bunch of lines way over that and it was impacting the score for something so trivial that we could fix over time and didn't impact the actual code or it's readability for the most part.

@envygeeks

envygeeks Mar 19, 2016

Contributor

@parkr the bot (and I believe Rubocop too) doesn't track line width on Jekyll/Jekyll for now because we have a bunch of lines way over that and it was impacting the score for something so trivial that we could fix over time and didn't impact the actual code or it's readability for the most part.

lib/jekyll/tags/link.rb
+ end
+
+ raise ArgumentError.new <<-eos
+Could not find document "#{@relative_path}" in tag '#{TagName}'.

This comment has been minimized.

@envygeeks

envygeeks Mar 17, 2016

Contributor
  • Use ArgumentError, "string".

Also:

raise ArgumentError, "This is my long string\n" \
  "This is the second part."
@envygeeks

envygeeks Mar 17, 2016

Contributor
  • Use ArgumentError, "string".

Also:

raise ArgumentError, "This is my long string\n" \
  "This is the second part."

This comment has been minimized.

@jeffkole

jeffkole Mar 17, 2016

Contributor

I was following the convention set in post_url.rb. Has the style changed?

@jeffkole

jeffkole Mar 17, 2016

Contributor

I was following the convention set in post_url.rb. Has the style changed?

This comment has been minimized.

@envygeeks

envygeeks Mar 17, 2016

Contributor

For removing the "new" yes. For the former, it's up for debate @jekyll/core will have to decide that though as of late it seems a lot of errors are shifting that way because it's cleaner to read.

@envygeeks

envygeeks Mar 17, 2016

Contributor

For removing the "new" yes. For the former, it's up for debate @jekyll/core will have to decide that though as of late it seems a lot of errors are shifting that way because it's cleaner to read.

test/test_tags.rb
+ end
+
+ should "have the url to the \"site/generate\" item" do
+ assert_match %r{2\s/methods/site/generate}, @result

This comment has been minimized.

@parkr

parkr Mar 24, 2016

Member

Should these have file extensions?

@parkr

parkr Mar 24, 2016

Member

Should these have file extensions?

+{% link non-existent-collection-item %}
+CONTENT
+
+ assert_raises ArgumentError do

This comment has been minimized.

@parkr

parkr Mar 24, 2016

Member

Note to self: change this in #4674.

@parkr

parkr Mar 24, 2016

Member

Note to self: change this in #4674.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 24, 2016

Member

Thanks, @jeffkole. One comment above about the tests then I'm 👍 to 🚢.

Member

parkr commented Mar 24, 2016

Thanks, @jeffkole. One comment above about the tests then I'm 👍 to 🚢.

@parkr parkr modified the milestones: flexible, undetermined Mar 24, 2016

@jeffkole

This comment has been minimized.

Show comment
Hide comment
@jeffkole

jeffkole Mar 24, 2016

Contributor

Thanks for the review @parkr and @envygeeks. I think this should be good to go now.

Contributor

jeffkole commented Mar 24, 2016

Thanks for the review @parkr and @envygeeks. I think this should be good to go now.

@parkr parkr changed the title from Adds collection_tag to Adds `link` Liquid tag to make generation of URL's easier Mar 24, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 24, 2016

Member

@jekyllbot: merge +minor

Member

parkr commented Mar 24, 2016

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit da4a664 into jekyll:master Mar 24, 2016

1 check passed

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

jekyllbot added a commit that referenced this pull request Mar 24, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 24, 2016

Member

Thanks a lot, @jeffkole!

Member

parkr commented Mar 24, 2016

Thanks a lot, @jeffkole!

@parkr parkr removed the pending-feedback label Mar 24, 2016

@parkr parkr modified the milestones: 3.2, flexible Mar 24, 2016

@jeffkole jeffkole deleted the jeffkole:feature/add-collection-url-tag branch Mar 24, 2016

parkr added a commit that referenced this pull request Mar 26, 2016

Merge remote-tracking branch 'origin/master' into themes
* origin/master: (65 commits)
  Update history to reflect merge of #4703 [ci skip]
  Update history to reflect merge of #4712 [ci skip]
  Highlight the test code
  Update history to reflect merge of #4640 [ci skip]
  readded "env=prod"-condition
  Update history to reflect merge of #3849 [ci skip]
  Update history to reflect merge of #4624 [ci skip]
  Update history to reflect merge of #4704 [ci skip]
  Update history to reflect merge of #4706 [ci skip]
  Checks for link file extension in tests
  Updating assets documentation
  Fix test teardown for cleaner.
  Update history to reflect merge of #4542 [ci skip]
  Add explanation of site variables in the example _config.yml
  Use double quotes in the gemfile
  Add test for creation of Gemfile by 'jekyll new'
  Add comment about github-pages
  Update history to reflect merge of #4533 [ci skip]
  Ensure Rouge closes its div/figure properly after highlighting ends.
  Add Site#config= which can be used to set the config
  ...
@kainjow

This comment has been minimized.

Show comment
Hide comment
@kainjow

kainjow Aug 1, 2016

Contributor

Should this tag be documented on the website? It's mentioned in the release notes for 3.2.

Contributor

kainjow commented Aug 1, 2016

Should this tag be documented on the website? It's mentioned in the release notes for 3.2.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 2, 2016

Member

@kainjow Huh. Yeah, it should be. It should be documented like post_url is, probably at the top of that list.

Member

parkr commented Aug 2, 2016

@kainjow Huh. Yeah, it should be. It should be documented like post_url is, probably at the top of that list.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Aug 2, 2016

Member

@jeffkole A few questions regarding the current implementation:

  1. Should we be able to link to a page with link?
  2. Could we keep the same behavior as post_url and omit the file extension in the relative path?
Member

DirtyF commented Aug 2, 2016

@jeffkole A few questions regarding the current implementation:

  1. Should we be able to link to a page with link?
  2. Could we keep the same behavior as post_url and omit the file extension in the relative path?
@jeffkole

This comment has been minimized.

Show comment
Hide comment
@jeffkole

jeffkole Aug 2, 2016

Contributor

@DirtyF You should be able to link to any document that Jekyll is generating, including pages. Is that not working?

As for the file extension, it could certainly be done. I'll leave it to @parkr for guidance, since that kind of stylistic decision is more up to him than me. If he likes it, I can make the changes.

Contributor

jeffkole commented Aug 2, 2016

@DirtyF You should be able to link to any document that Jekyll is generating, including pages. Is that not working?

As for the file extension, it could certainly be done. I'll leave it to @parkr for guidance, since that kind of stylistic decision is more up to him than me. If he likes it, I can make the changes.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 2, 2016

Member

Could we keep the same behavior as post_url and omit the file extension in the relative path?

@DirtyF What's the reason for this?

Member

parkr commented Aug 2, 2016

Could we keep the same behavior as post_url and omit the file extension in the relative path?

@DirtyF What's the reason for this?

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Aug 2, 2016

Member

@jeffkole I didn't manage to link to pages when testing for #5182

@parkr just for cohesiveness between post_url and linktags. IMHO as a user it's not intuitive to have difference between both syntax.

Member

DirtyF commented Aug 2, 2016

@jeffkole I didn't manage to link to pages when testing for #5182

@parkr just for cohesiveness between post_url and linktags. IMHO as a user it's not intuitive to have difference between both syntax.

jeffkole added a commit to jeffkole/jekyll that referenced this pull request Aug 3, 2016

Adds ability to link to all files
Fixes request made in #4624 and bug found in #5182
@jeffkole

This comment has been minimized.

Show comment
Hide comment
@jeffkole

jeffkole Aug 3, 2016

Contributor

Opened #5199 to link to pages and static files.

Contributor

jeffkole commented Aug 3, 2016

Opened #5199 to link to pages and static files.

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