Add JekyllPageGenerator #10
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Includes tests.
Jekyll is the underlying technology used for GitHub Pages. Jekyll parses markdown and liquid files to HTML pages for a site. Generally Jekyll expects "frontmatter" - a `---` delimited block at the top of the markdown file -. The existence of the frontmatter indicates to Jekyll that the page should be _processed_ by the transformation engine and not just copied over. Frontmatter also allows for adding variables to the page which can be used in the page/theme to do certain things. Even though for GitHub Pages, a plugin is active which make the frontmatter not _strictly_ necessary, for this "PHPCS docs" type of website it is useful. By default, the first `#` (H1) header will be regarded as the `page.title` and this page title is then subsequently used in the website menu and such. As for these sniff pages, the default title is the full sniffname `Standard.Category.SniffName` and the `Standard` and `Category` are already "levels" in a typical menu due to the folder structure, it is less noisy to use just the plain `SniffName` as the `page.title`. To that end, I'm adding a separate `JekyllPageGenerator` class which extends the standard `MarkdownGenerator` class and adds the _frontmatter_ to the page. This will also allow for extending the available frontmatter with additional keys in the future if deemed necessary. Includes test.
afilina
reviewed
Mar 18, 2021
|
|
||
| use App\Value\Sniff; | ||
|
|
||
| class JekyllPageGenerator extends MarkdownGenerator implements Generator |
There was a problem hiding this comment.
Classes should be final and we should not extend implementations. It's okay to copy an existing to make the necessary changes. We can always extract truly reusable elements and inject them as dependencies instead. The test for the new class should encompass the common elements as well, otherwise we're not truly testing them, and any unintended regressions would go unnoticed. Not a deal breaker and I'm happy to address the issue in a subsequent commit.
There was a problem hiding this comment.
Let's defer it and create a ticket to improve later.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sniff: add getter for the standard, category and sniff name
Includes tests.
Add JekyllPageGenerator
Jekyll is the underlying technology used for GitHub Pages. Jekyll parses markdown and liquid files to HTML pages for a site.
Generally Jekyll expects "frontmatter" - a
---delimited block at the top of the markdown file -.The existence of the frontmatter indicates to Jekyll that the page should be processed by the transformation engine and not just copied over.
Frontmatter also allows for adding variables to the page which can be used in the page/theme to do certain things.
Even though for GitHub Pages, a plugin is active which make the frontmatter not strictly necessary, for this "PHPCS docs" type of website it is useful.
By default, the first
#(H1) header will be regarded as thepage.titleand this page title is then subsequently used in the website menu and such.As for these sniff pages, the default title is the full sniffname
Standard.Category.SniffNameand theStandardandCategoryare already "levels" in a typical menu due to the folder structure, it is less noisy to use just the plainSniffNameas thepage.title.To that end, I'm adding a separate
JekyllPageGeneratorclass which extends the standardMarkdownGeneratorclass and adds the frontmatter to the page.This will also allow for extending the available frontmatter with additional keys in the future if deemed necessary.
Includes test.