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

Add different feed providers #379

Closed
wants to merge 4 commits into from
Closed

Add different feed providers #379

wants to merge 4 commits into from

Conversation

gabor-meszaros
Copy link
Contributor

Solution for #378 . Could you please review it? I am quite new in Jekyll and Liquid. :) Thank you!

@mmistakes
Copy link
Owner

mmistakes commented Jun 25, 2016

I'm onboard with this enhancement. One small suggestion. What of instead of doing something similar to the comments provider customization, we go with something much simpler:

  1. By default use feed.xml that builds with the jekyll-feed plugin.
  2. All the default feed.xml to be overriden by specifying a path to an external one eg: http://feeds.feedburner.com/whatever or anywhere else.

Removing the provider stuff makes it easier to use any other service.

Thinking _config.yml would look something like

feed:
  path: # blank (default) uses feed.xml

And _includes/footer.html

  <li><a href="{% if site.feed.path %}{{ site.feed.path }}{% else %}{{ base_path }}/feed.xml{% endif %}"><i class="fa fa-fw fa-rss-square" aria-hidden="true"></i> {{ site.data.ui-text[site.locale].feed_label }}</a></li>

Followed the recommendation of @mmistakes. Link:
#379
@gabor-meszaros
Copy link
Contributor Author

Thank you for the review! It makes sense and indeed less complicated than my original solution.

I did the modifications. Is it anything else that I can do to improve it?

@@ -29,9 +29,7 @@ comments:
num_posts : # 5 (default)
colorscheme : # "light" (default), "dark"
feed:
Copy link
Owner

Choose a reason for hiding this comment

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

Testing this I hit an error. Maybe change feed to atom_feed since I believe the previous name was conflicting with the jekyll-feed plugin.

Also if you could squash all the commits into 1 with rebase that would make things cleaner when I merge the pull request in.

Thanks!

@gabor-meszaros
Copy link
Contributor Author

Thank you for the suggestions! I am sorry for the quite late answer...

I noticed that some commits were added to the repository since my last commit, I am not sure if I can do a rebase in this situation. If it is possible, could you please give me some hints about how to do it?

I do not want to hack around if there is a better solution to this, but I can fork your repo again, do the modification, and create a new PR if that makes the git log more readable.

What is your opinion? Thanks!

@mmistakes
Copy link
Owner

Rebasing and flattening everything down into a single commit would probably be cleaner. But I'm not the best at Git and haven't had much luck rebasing myself.

If you don't mind, probably easier to just refork and open a new PR.

@gabor-meszaros
Copy link
Contributor Author

Sure, I will do it right now. Thanks for your patience!

@gabor-meszaros
Copy link
Contributor Author

I added the new PR. Could you please review it whether it is OK? Thanks.

@mmistakes mmistakes closed this in 2cfad13 Jul 26, 2016
mmistakes added a commit that referenced this pull request Jul 26, 2016
cjmadsen pushed a commit to cjmadsen/cjmadsen.github.io that referenced this pull request Dec 7, 2016
jluccisano added a commit to jluccisano/jluccisano.github.io that referenced this pull request May 6, 2017
makaroniame added a commit to makaroniame/makaroniame-old.github.io that referenced this pull request May 18, 2022
koyumi0601 pushed a commit to koyumi0601/koyumi0601.github.io that referenced this pull request Jul 31, 2023
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.

2 participants