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

`include_relative` tag should find related documents in collections gathered within custom `collections_dir` #6818

Merged
merged 4 commits into from Apr 12, 2018

Conversation

Projects
None yet
7 participants
@ashmaroli
Member

ashmaroli commented Mar 1, 2018

Resolves #6816

/cc @hosnas

@ashmaroli ashmaroli added this to the v3.8.0 milestone Mar 4, 2018

@ashmaroli ashmaroli requested review from parkr, pathawks and oe Mar 11, 2018

@DirtyF

DirtyF approved these changes Mar 14, 2018

And the "_site/puppies/rover.html" file should exist
And I should see "Random Content." in "_site/2009/03/27/gathered-post.html"
Scenario: Rendered collection in custom collections_dir with a document that includes a relative

This comment has been minimized.

@oe

oe Mar 14, 2018

Member

a relative what? is the line intentionally broken off here?

This comment has been minimized.

@ashmaroli

ashmaroli Mar 14, 2018

Member

um.. that was intended to be the end of line..
For comparison:

"She's a relative"
"Guests may include a relative"

This comment has been minimized.

@ashmaroli

ashmaroli Mar 14, 2018

Member

(I'm open to change it to a better phrase)

This comment has been minimized.

@oe

oe Mar 14, 2018

Member

a related document or something?

@hosnas

This comment has been minimized.

hosnas commented Mar 17, 2018

This is just a bugfix. can we have it sooner than v3.8.0 ?

@DirtyF DirtyF added the fix label Mar 17, 2018

site = context.registers[:site]
page_payload = context.registers[:page]
resource_path = \
if page_payload.is_a?(Jekyll::Drops::DocumentDrop)

This comment has been minimized.

@parkr

parkr Mar 20, 2018

Member

What do you think about !page_payload[“collection”].nil? instead of checking the class name? That would be more future proof :)

This comment has been minimized.

@ashmaroli

ashmaroli Mar 20, 2018

Member

Awesome! I like that suggestion..

When I run jekyll build
Then I should get a zero exit status
And the _site directory should exist
And the "_site/puppies/rover.html" file should exist

This comment has been minimized.

@parkr

parkr Mar 20, 2018

Member

Shouldn’t you check that this contains “excerpt for all docs.”?

When I run jekyll build
Then I should get a zero exit status
And the _site directory should exist
And the "_site/puppies/rover.html" file should exist

This comment has been minimized.

@parkr

parkr Mar 20, 2018

Member

Same here - validate that the tag is working properly by checking file contents?

@pathawks

This comment has been minimized.

Member

pathawks commented Mar 20, 2018

https://github.com/ashmaroli/jekyll/blob/6f3a3c1cb09bd8649bd7cb3b7b5293a643346144/lib/jekyll/tags/include.rb#L219-L223

It's too bad there's not a more orthogonal way to get the path of a document, whether or not it is in a collection. 🤷‍♂️

@pathawks

This comment has been minimized.

Member

pathawks commented Apr 12, 2018

@jekyllbot: 🚢 +bug

@jekyllbot jekyllbot merged commit 651b9b5 into jekyll:master Apr 12, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot added bug fix labels Apr 12, 2018

@ashmaroli ashmaroli deleted the ashmaroli:include-relative-collection-dir branch Apr 12, 2018

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