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

Output feed links for collections #254

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

goulvench
Copy link

Makes {% feed_meta %} generate a link for every collection.

All meta links have the same title at the moment, which is not good. But if my previous PR is accepted, it's just a matter of replacing :title => title, with :title => generator.feed_title(:collection => collection, :category => category) (and removing the title method from meta-tag.rb).

If there's a better way of grabbing a reference to the generator from meta-tag, I'd love to hear about it.

@goulvench
Copy link
Author

goulvench commented Nov 14, 2018

I'm open to discussion regarding this PR because it is a breaking change (an improvement in most cases, but still). Should feed_meta use parameters to make the new behavior opt-in?
Proposed syntax:

# Passing the collection name, requires one line per collection
{% feed_meta "updates" %}

# A boolean flag, if included all collections are listed
{% feed_meta including_collections %}

# Allow selecting which collections are included
{% feed_meta including_collections: "updates photos" %}

Feel free to give your input on this, I'd be happy to implement it and update the PR.

@goulvench
Copy link
Author

goulvench commented Nov 17, 2018

Since I need this, I went ahead and made the helper collection- and category-aware.

For full backwards compatibility, the new possibilities are all 'opt-in'.

# Without parameters, generates link to the default "posts" collection, as before
{% feed_meta %}

# With a collection name, generates link to that collection
{% feed_meta my_collection %}

# With a collection and category name, generates link to that collection/category
{% feed_meta my_collection my_category %}

# With the special include: all argument, generates links to all categories and posts
{% feed_meta include: all %}

I'll merge this and feed title PRs in my repo so you can try it out —and I can start using it right away. Feel free to give me some feedback if you have any!

@goulvench goulvench force-pushed the output-feed-links-for-collections branch from f3941ca to 4d58045 Compare January 21, 2019 09:54
@goulvench
Copy link
Author

I've updated my pull request to include the latest changes from jekyll-feed master.

@goulvench
Copy link
Author

Hi there, is there anything blocking this PR from being merged? I'll gladly update it if needed, just let me know what needs to be improved. Thanks!

Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

Needs some changes..

lib/jekyll-feed/meta-tag.rb Outdated Show resolved Hide resolved
lib/jekyll-feed/meta-tag.rb Show resolved Hide resolved
lib/jekyll-feed/meta-tag.rb Show resolved Hide resolved
lib/jekyll-feed/meta-tag.rb Outdated Show resolved Hide resolved
lib/jekyll-feed/meta-tag.rb Show resolved Hide resolved
Thanks @ashmaroli for spotting that one!

Co-Authored-By: goulvench <goulvench@gmail.com>
@ashmaroli
Copy link
Member

@goulvench Thank you very much for addressing all of my concerns.
Just one final hurdle:

I recently learned that YAML parses yes || YES as boolean true and no || NO as boolean false.
How do we handle the following unlikely but probable configuration:

feed:
  collections:
    collection:
      categories:
        - no
        - false

@DirtyF
Copy link
Member

DirtyF commented Feb 6, 2019

@ashmaroli That's an edge case, can we address this in a specific issue, but it's how YAML works, so best to avoid naming your categories as boolean. 😅

ashmaroli
ashmaroli previously approved these changes Feb 6, 2019
Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@ashmaroli ashmaroli requested a review from a team February 6, 2019 15:44
@DirtyF DirtyF added this to the Next minor version milestone Nov 13, 2019
@ashmaroli
Copy link
Member

This needs to have conflicts resolved..

@goulvench
Copy link
Author

Hi, I'd love to see this land in core but it's been more than 20 months since I opened this PR and I don't have time to work on it at the moment. Feel free to implement this in some way, fix the conflicts yourselves, or simply close this PR.

@ashmaroli ashmaroli dismissed their stale review December 16, 2021 09:11

Dismissing approval due to complex conflicts

@ashmaroli ashmaroli removed the request for review from a team December 16, 2021 09:12
@ashmaroli ashmaroli removed this from the Next minor version milestone Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants