Refactor and add sitemap to site.pages
#137
Conversation
pathawks
force-pushed
the
pr/refactor
branch
4 times, most recently
from
509e22f
to
8ff1599
Compare
| INCLUDED_EXTENSIONS = %W( | ||
| .htm | ||
| .html | ||
| .xhtml | ||
| ).freeze | ||
|
|
||
| MINIFY_REGEX = %r!(>\n|})\s+!.freeze |
There was a problem hiding this comment.
If we see a > followed by a single linebreak, or a }, we will strip all the whitespace that follows.
There was a problem hiding this comment.
Can you add that explanation as a code comment?
| INCLUDED_EXTENSIONS = %W( | ||
| .htm | ||
| .html | ||
| .xhtml | ||
| ).freeze | ||
|
|
||
| MINIFY_REGEX = %r!(>\n|})\s+!.freeze |
There was a problem hiding this comment.
Can you add that explanation as a code comment?
| end | ||
|
|
||
| private |
There was a problem hiding this comment.
Technically, technically, this would be a breaking change to the Ruby API. I'm all for it, but we'd have to make it a major bump then.
There was a problem hiding this comment.
You are correct, but since we have not yet reached 1.0.0, a minor bump will suffice.
From semver.org:
Version 1.0.0 defines the public API. The way in which the version number is incremented after this release is dependent on this public API and how it changes.
So, technically, we have not yet defined a public API.
| def write | ||
| FileUtils.mkdir_p File.dirname(destination_path) | ||
| File.open(destination_path, "w") { |f| f.write(sitemap_content) } | ||
| @site.in_dest_dir("sitemap.xml") | ||
| end | ||
|
|
||
| def sitemap_content |
There was a problem hiding this comment.
Does this return the content, or a Page, if so, perhaps just sitemap?
|
I don't know if it's worth documenting, but this change adds the sitemap to |
pathawks
force-pushed
the
pr/refactor
branch
2 times, most recently
from
a84e934
to
5a0e199
Compare
pathawks
dismissed
benbalter’s stale review
I am nervous and want to have one more review with the changes I made, please
|
@benbalter Could you look over once more please? |
| @site.keep_files ||= [] | ||
| @site.keep_files << "sitemap.xml" | ||
| end | ||
| @site.pages << sitemap unless sitemap_exists? |
There was a problem hiding this comment.
This may cause a problem for sites that iterate through pages, but if we call it out in the changelog, we should be fine.
| @@ -22,6 +22,10 @@ those other gems if you *want* the sitemap to include the generated | |||
| content, or *before* those other gems if you *don't want* the sitemap to | |||
| include the generated content from the gems. (Programming is *hard*.) | |||
|
|
|||
| Because the sitemap is added to `site.pages`, you may have to modify any | |||
| templates that iterate through all pages (for example, to build a menu of | |||
| all of the site's content). | |||
There was a problem hiding this comment.
@benbalter Is this enough of an explanation?
|
@jekyllbot: merge |
This PR takes advantage of some of the newer features in Jekyll to reduce some of the code necessary. Now that we require Jekyll 3.3, we do not need to keep all the old workarounds.