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

No layouts for excerpt #1339

Merged
merged 16 commits into from Jul 24, 2013

Conversation

Projects
None yet
5 participants
@parkr
Member

parkr commented Jul 22, 2013

Aims to fix #1321.

  • Don't put excerpts in layouts
  • Convert layouts with converter
  • Render liquid
  • Write tests so it never happens again
  • Fix shitty tests so it really never happens again
@@ -249,12 +252,13 @@ def render(layouts, site_payload)
# construct payload
payload = {
"site" => { "related_posts" => related_posts(site_payload["site"]["posts"]) },
"page" => self.to_liquid
"page" => self.to_liquid(EXCERPT_ATTRIBUTES_FOR_LIQUID)

This comment has been minimized.

@parkr

parkr Jul 22, 2013

Member

@mattr- The key to get it to convert was to make sure Post#excerpt was not called before the excerpt was converted. This seems to fix all the things!

@parkr

parkr Jul 22, 2013

Member

@mattr- The key to get it to convert was to make sure Post#excerpt was not called before the excerpt was converted. This seems to fix all the things!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 22, 2013

Member

@benbalter this will likely be my last PR for v1.1.1 so we can get it shipped today or (at the very latest) tomorrow.

Member

parkr commented Jul 22, 2013

@benbalter this will likely be my last PR for v1.1.1 so we can get it shipped today or (at the very latest) tomorrow.

Show outdated Hide outdated features/post_excerpts.feature
@@ -0,0 +1,50 @@
Feature: Post excerpts
As a hacker who likes to blog
I want to be able to make a static sitej

This comment has been minimized.

@benbalter

benbalter Jul 22, 2013

Contributor

s/sitej/site/

@benbalter

benbalter Jul 22, 2013

Contributor

s/sitej/site/

@@ -15,6 +15,16 @@ class Stevenson
def initialize(level = INFO)
@log_level = level
end
# Public: Print a jekyll debug message to stdout

This comment has been minimized.

@benbalter

benbalter Jul 22, 2013

Contributor

Is this intended to be in this pull request? Don't see debug called at all.

@benbalter

benbalter Jul 22, 2013

Contributor

Is this intended to be in this pull request? Don't see debug called at all.

This comment has been minimized.

@parkr

parkr Jul 22, 2013

Member

I used it in a couple places while debugging but opted to take those out. Before we can have log levels, we need to come up with useful output.

@parkr

parkr Jul 22, 2013

Member

I used it in a couple places while debugging but opted to take those out. Before we can have log levels, we need to come up with useful output.

This comment has been minimized.

@parkr

parkr Jul 22, 2013

Member

But I figured if I wrote the method, I may as well include it for later use.

@parkr

parkr Jul 22, 2013

Member

But I figured if I wrote the method, I may as well include it for later use.

Given I have an "index.html" page that contains "{% for post in site.posts %}{{ post.excerpt }}{% endfor %}"
And I have a _posts directory
And I have a _layouts directory
And I have a post layout that contains "{{ page.excerpt }}"

This comment has been minimized.

@mattr-

mattr- Jul 22, 2013

Member

should this be {{ post.excerpt }} for consistency?

@mattr-

mattr- Jul 22, 2013

Member

should this be {{ post.excerpt }} for consistency?

This comment has been minimized.

@parkr

parkr Jul 23, 2013

Member

No, the variable used in the layout is page, not post

@parkr

parkr Jul 23, 2013

Member

No, the variable used in the layout is page, not post

This comment has been minimized.

@mattr-

mattr- Jul 23, 2013

Member

😕 Why is it page and not post? Seems weird to me.

@mattr-

mattr- Jul 23, 2013

Member

😕 Why is it page and not post? Seems weird to me.

This comment has been minimized.

@parkr

parkr Jul 23, 2013

Member

It's to be consistent so you can use the same layout for pages and for posts if you want.

Always the same variable for the #to_liquid hash

@parkr

parkr Jul 23, 2013

Member

It's to be consistent so you can use the same layout for pages and for posts if you want.

Always the same variable for the #to_liquid hash

This comment has been minimized.

@mattr-

mattr- Jul 24, 2013

Member

Ah, got it. Merging shortly.

@mattr-

mattr- Jul 24, 2013

Member

Ah, got it. Merging shortly.

Show outdated Hide outdated lib/jekyll/excerpt.rb
# Returns Hash of post data
def data
@data ||= post.data.dup
@data.delete("layout") if @data.has_key?("layout")

This comment has been minimized.

@mattr-

mattr- Jul 22, 2013

Member

Just @data.delete("layout") is fine here. If the key does not exist, we'll get the default value.

@mattr-

mattr- Jul 22, 2013

Member

Just @data.delete("layout") is fine here. If the key does not exist, we'll get the default value.

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Jul 22, 2013

Member

Just the one fixup and I think this should be good.

Member

mattr- commented Jul 22, 2013

Just the one fixup and I think this should be good.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 23, 2013

Member

@mattr- Please merge when ready :)

Member

parkr commented Jul 23, 2013

@mattr- Please merge when ready :)

@parkr parkr referenced this pull request Jul 23, 2013

Merged

Minor refactors #1341

mattr- added a commit that referenced this pull request Jul 24, 2013

@mattr- mattr- merged commit d68d29c into master Jul 24, 2013

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request Jul 24, 2013

@parkr parkr deleted the no-layouts-for-excerpt branch Jul 24, 2013

@nitoyon

This comment has been minimized.

Show comment
Hide comment
@nitoyon

nitoyon Sep 22, 2013

Contributor

Because Array#concat is destructive, ATTRIBUTES_FOR_LIQUID and EXCERPT_ATTRIBUTES_FOR_LIQUID refers to the same array.

irb(main):001:0> a = [1]
=> [1]
irb(main):002:0> a.concat [2]
=> [1, 2]
irb(main):003:0> a
=> [1, 2]

You should write as following:

ATTRIBUTES_FOR_LIQUID = EXCERPT_ATTRIBUTES_FOR_LIQUID + %w[
  content
  excerpt
]
Contributor

nitoyon commented on lib/jekyll/post.rb in 26dc148 Sep 22, 2013

Because Array#concat is destructive, ATTRIBUTES_FOR_LIQUID and EXCERPT_ATTRIBUTES_FOR_LIQUID refers to the same array.

irb(main):001:0> a = [1]
=> [1]
irb(main):002:0> a.concat [2]
=> [1, 2]
irb(main):003:0> a
=> [1, 2]

You should write as following:

ATTRIBUTES_FOR_LIQUID = EXCERPT_ATTRIBUTES_FOR_LIQUID + %w[
  content
  excerpt
]

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 22, 2013

Member

Bollocks, you're right. It doesn't look like it's a problem so far, but I'll fix that per this info. Thanks!

Member

parkr replied Sep 22, 2013

Bollocks, you're right. It doesn't look like it's a problem so far, but I'll fix that per this info. Thanks!

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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