Skip to content
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

Excerpt only flag #287

Merged
merged 4 commits into from
Nov 7, 2019
Merged

Excerpt only flag #287

merged 4 commits into from
Nov 7, 2019

Conversation

torrocus
Copy link
Contributor

@torrocus torrocus commented Oct 7, 2019

Create new flag excerpt_only, which allows to exclude post content from feed.

PR to issue: #277

BTW I don't like name excerpt_only, but don't have better idea. IMO this name should get true value for full content and false value for excerpt.

@torrocus torrocus mentioned this pull request Oct 7, 2019
Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dig this!

@ashmaroli
Copy link
Member

@torrocus First of all, apologies for not reviewing this earlier.
While I don't have an issue with the idea, I'm on the fence regarding the implementation.

Since Parker has already approved this, I won't block this from being merged but would love it if you could clarify the following / make some changes:

  • | default: false }} is unnecessary. The subsequent conditional is valid even if excerpt_only evaluates to nil.
  • Why conditionally assign post_content and then immediately test post_content for a truthy? Why not render directly?
    {% unless excerpt_only %}
      <content type="html" xml:base="{{ post.url | absolute_url | xml_escape }}">
        {{ post.content | strip | xml_escape }}
      </content>
    {% endunless %}

@torrocus
Copy link
Contributor Author

torrocus commented Nov 4, 2019

@parkr Thanks for dig.
@ashmaroli No problem, it's good to know someone is coming here.

Regarding your attention with | default: false }} you're right. I deleted the redundant default value. I wanted to show what the default value is. But later I included this information in the documentation and it should be enough.

Your second point is not that simple. Post does not always have content. The question is is this correct? View to file spec/fixtures/_posts/2015-02-12-strip-newlines.md
Personally, I would prefer render directly. But I do not know the whole context and did not want to interfere with existing functionalities.

@ashmaroli
Copy link
Member

Post does not always have content

To my knowledge, the content attribute is never nil or anything other than a String. It can be just empty "" but in Ruby, that is considered a truthy as well.

Of course, having a test for this would make things clear and concrete.

@torrocus
Copy link
Contributor Author

torrocus commented Nov 4, 2019

Right, I corrected it. Previously, one test failed. Now everything is OK, and I already had this code written in this way.

@torrocus
Copy link
Contributor Author

torrocus commented Nov 4, 2019

Please squash all commits when you will merge.

@ashmaroli
Copy link
Member

@parkr Since the diff changed significantly after you gave your approval, could you do a re-review?
Thanks.

@DirtyF DirtyF requested a review from parkr November 7, 2019 10:41
Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@DirtyF
Copy link
Member

DirtyF commented Nov 7, 2019

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 8bb2bb0 into jekyll:master Nov 7, 2019
jekyllbot added a commit that referenced this pull request Nov 7, 2019
@torrocus torrocus deleted the excerpt-only-flag branch November 12, 2019 12:40
DirtyF added a commit that referenced this pull request Nov 13, 2019
Minor enhancements:

  * Excerpt only flag (#287)
  * Add media:content tag (#290)

Fix #283
@jekyllbot jekyllbot mentioned this pull request Nov 13, 2019
@jekyll jekyll locked and limited conversation to collaborators Nov 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants