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.

Sign up for GitHub

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

Jump to bottom

feat: remove 404 pages from the sitemap. closes #113 #164

Merged
merged 4 commits into from Mar 24, 2017
Merged

feat: remove 404 pages from the sitemap. closes #113 #164

merged 4 commits into from Mar 24, 2017

Conversation

ghost
Copy link

@ghost ghost commented Mar 24, 2017 •
edited by ghost
Loading

poof. fixes #113.

@ghost ghost mentioned this pull request Mar 24, 2017
Closed
pathawks
pathawks requested changes Mar 24, 2017
View reviewed changes
Copy link
Member

@pathawks pathawks left a comment

Choose a reason for hiding this comment

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

Could also be a Document, in addition to a static file, right?

@ghost
Copy link
Author

ghost commented Mar 24, 2017 •
edited by ghost
Loading

Could also be a Document, in addition to a static file, right?

@pathawks Sorry, something's up with GitHub and I'm not able to respond directly to your review comment. Do you mean 404.md? If so, all markdown files are converted to HTML when output in the sitemap and testing that conversion is outside the scope of this plug-in or test.

benbalter
benbalter approved these changes Mar 24, 2017
View reviewed changes
Copy link
Contributor

@benbalter benbalter left a comment

Choose a reason for hiding this comment

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

Nice! 👾 ♠️ 🍝

@ghost
Copy link
Author

ghost commented Mar 24, 2017

I do what I can.

@pathawks
Copy link
Member

@JHabdas Sorry, I was confused.

What I meant is that 404.html could be a static file (without front matter), so we should probably also check in the third loop.

It is probably unlikely that somebody is outputting 404.html from a collection. (Somebody will quote this line in an issue a year from now, I am sure. 😛)

@benbalter
Copy link
Contributor

I believe {% for file in page.static_files %} needs the same logic, in case the 404 file doesn't have YAML front matter.

@ghost
Copy link
Author

ghost commented Mar 24, 2017

@benbalter oic. on it

@ghost
Copy link
Author

ghost commented Mar 24, 2017

@pathawks @benbalter thanks for ensuring excellence in your work. I've added the missing fixture and test. you guys have eagle eyes, I swear.

pathawks
pathawks reviewed Mar 24, 2017
View reviewed changes
lib/sitemap.xml Outdated
{% for file in page.static_files %}
{% assign static_files = page.static_files | where_exp:'page','page.name != "404.html"' %}
{% for file in static_files %}
{{ page.path }}
Copy link
Member

Choose a reason for hiding this comment

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

We probably don't need to output the path here.

Copy link
Author

Choose a reason for hiding this comment

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

oops

@pathawks
Copy link
Member

One comment. Otherwise LGTM

Thanks for getting this done 🍻🎉

@ghost
Copy link
Author

ghost commented Mar 24, 2017

@pathawks all set with a02af55

pathawks
pathawks reviewed Mar 24, 2017
View reviewed changes
spec/jekyll-sitemap_spec.rb Outdated
@@ -93,10 +93,14 @@
expect(contents).to match /\/some-subfolder\/htm\.htm/
end

it "does include assets or any static files with .pdf extension" do
it "does include assets any static files with .pdf extension" do
Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed?

Copy link
Author

Choose a reason for hiding this comment

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

I'm sorry, @pathawks restoring

@pathawks
Copy link
Member

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 433c0de into jekyll:master Mar 24, 2017
jekyllbot added a commit that referenced this pull request Mar 24, 2017
@jekyllbot
Update history to reflect merge of #164 [ci skip]
b5088b2
@ghost ghost deleted the 404-begone branch March 24, 2017 19:02
@pathawks
Copy link
Member

@JHabdas Thank you so much! I appreciate your patience.

@DirtyF
Copy link
Member

DirtyF commented Mar 24, 2017

No more need for sitemap: false in 404.html 🎉

@mmistakes mmistakes mentioned this pull request May 4, 2017
Closed
@jekyll jekyll locked and limited conversation to collaborators Apr 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement frozen-due-to-age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

404.html appearing in sitemap.xml
4 participants
@pathawks @benbalter @DirtyF @jekyllbot