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

Broken link to /feed.xml out-of-the-box. #98

Closed
gsnedders opened this Issue Jan 4, 2017 · 15 comments

Comments

Projects
None yet
8 participants
@gsnedders
Copy link

gsnedders commented Jan 4, 2017

Out of the box, minima produces a broken link to /feed.xml, because it requires jekyll-feed. Minima should either depend on jekyll-feed or not output the link to the feed.

@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Jan 4, 2017

For reference, here is the offending line. We wouldn't want to use this with jekyll-feed anyway, since that generates Atom rather than application/rss+xml.

@DirtyF

This comment has been minimized.

Copy link
Member

DirtyF commented Jan 4, 2017

@pathawks I was thinking of adding jekyll-feed as a runtime dependency in the gemspec, modify example Gemfile and _config.yml and use feed meta instead in the include, does this seem OK to you?

On a side note, an html-proofer test of the output wouldn't hurt to prevent this type of error.

@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Jan 4, 2017

I seem to remember their being an intentional decision not to tie minima to jekyll-feed, as minima is supposed to be a minimal.

Can't find that thread now.

@gsnedders

This comment has been minimized.

Copy link
Author

gsnedders commented Jan 4, 2017

I for one would rather it not be a dependency, FWIW, especially for sites which don't have any posts (i.e., consist merely of pages and collections).

@ricalo

This comment has been minimized.

Copy link

ricalo commented Jan 4, 2017

I agree with the line of thought that minima should not include jekyll-feed. Feeds could be easily added by those who need it.

And agree with the html-proofer test too. That's how I ran into this problem myself.

Edit: For now, I'm just running my tests with the following command:

htmlproofer --url-ignore /feed.xml ./_site

ricalo added a commit to ricalo/JekyllDocker that referenced this issue Jan 4, 2017

Ignoring /feed.xml url in HtmlProofer test
Due to issue jekyll/minima#98 I'm ignoring this Html-Proofer error
@alzeih

This comment has been minimized.

Copy link

alzeih commented Jan 9, 2017

As a workaround, I used this in _layouts/home.html:

  {% if site.gems contains "jekyll-feed" %}
    <p class="rss-subscribe">subscribe <a href="{{ "/feed.xml" | relative_url }}">via RSS</a></p>
  {% endif %}
@alzeih

This comment has been minimized.

Copy link

alzeih commented Jan 27, 2017

There's (only) one more place that references feed.xml:

_includes/head.htmlcontains

<link rel="alternate" type="application/rss+xml" title="{{ site.title | escape }}" href="{{ "/feed.xml" | relative_url }}">

So perhaps this should be guarded with a check also?

@ashawley

This comment has been minimized.

Copy link
Contributor

ashawley commented May 29, 2017

If you use jekyll new, then you get the dependency for jekyll-feed:

See https://github.com/jekyll/jekyll/blob/af5d096/lib/site_template/_config.yml#L31

So someone just forgot to add that dependency when the theme was forked to this repo last year?

@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Aug 17, 2017

@DirtyF: I was thinking of adding jekyll-feed as a runtime dependency
@pathawks: ..an intentional decision not to tie minima to jekyll-feed as minima is supposed to be a minimal.

Is the above still valid, esp. since Minima will now come with jekyll-seo-tag included?

@alzeih

This comment has been minimized.

Copy link

alzeih commented Jan 16, 2018

I think minima should really be minimal, and not require jekyll-feed or jekyll-seo-tag (or any other jekyll plugins).

If that's not possible, I think it should at least degrade gracefully.

Maybe the default jekyll post could contain instructions (or a link to docs) for finding and installing plugins?
https://github.com/jekyll/jekyll/blob/master/lib/site_template/_posts/0000-00-00-welcome-to-jekyll.markdown.erb

@alzeih alzeih referenced this issue Jan 16, 2018

Closed

Remove /feed.xml #182

@ashawley

This comment has been minimized.

Copy link
Contributor

ashawley commented Jan 16, 2018

Yeah, hopefully the "degrade gracefully" fix in #129 gets merged since jekyll new currently sets the theme to minima and adds jekyll-feed.

@alzeih

This comment has been minimized.

Copy link

alzeih commented Jan 16, 2018

@ashawley: I've asked for the remove option in #182, for the reasons I've outlined there.

If this patch gets accepted, and the minima gem is released with this patch, then the Jekyll site_template/_config.yml should probably drop jekyll-feed too. However the extra plugin probably won't break anything, so if this takes a while to be removed from the _config.yml it shouldn't be a problem. (or left there as an example of how to use plugins).

jekyll-feed's documentation is fairly good, as is Jekyll's on plugins. I think the effort in adding a plugin and configuring it is less effort than obtaining, modifying and maintaining two minima template files in every jekyll app that uses minima for OOTB to not 404. That does depend on whether most users use RSS or not though.

Graceful degradation is limited by some technical reasons of Jekyll. The one's I can think of off hand are the multiple places plugins are enabled (site.gems, site.plugins, Gemfile, _plugins), and something to do with how liquid handles missing tags guarded by if checks still crashes. These seem like issues that are much harder to solve right now, but if these are fixed then I'd be absolutely for graceful degradation.

@ashawley

This comment has been minimized.

Copy link
Contributor

ashawley commented Jan 16, 2018

Last I checked, site.gems will work with Gemfile and is forwards compatible with the new site.plugins. But this should be taken up at #129 and discussed there. There's no disputing that minima is broken out of the box as this issue documents.

@alzeih

This comment has been minimized.

Copy link

alzeih commented Jan 16, 2018

Yep, you're right about site.gems and site.plugins. I've no idea how to query for plugins enabled in a Gemfile with the :jekyll-plugins group though.

@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Jan 16, 2018

I've no idea how to query for plugins enabled in a Gemfile with the :jekyll-plugins group though.

The fact is that you cannot query for plugins activated via :jekyll_plugins group..
Its a convenience block.. same as how Jekyll 3.5 onwards will automatically activate a theme's whitelisted runtime_dependencies

DirtyF added a commit that referenced this issue Jan 16, 2018

DirtyF added a commit that referenced this issue Jan 16, 2018

@DirtyF DirtyF closed this in #183 Jan 27, 2018

DirtyF added a commit that referenced this issue Jan 27, 2018

kusma added a commit to kusma/fuse-open.github.io that referenced this issue May 17, 2018

head.html: drop feed_meta
We're not doing a blog, so having a feed.xml-reference is just
pointless. Besides, we don't actually enable the plugin, so the
feed.xml-file isn't even generated.

This works around jekyll/minima#98.

@kusma kusma referenced this issue May 17, 2018

Merged

No feed meta #16

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