-
Notifications
You must be signed in to change notification settings - Fork 573
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 head-custom.html for custom head tags. #30
Conversation
Welcome! Congrats on your first pull request to the Midnight theme. If you haven't already, please be sure to check out the contributing guidelines. |
@@ -15,7 +15,7 @@ | |||
<link rel="stylesheet" href="{{ '/assets/css/ie.css' | relative_url }}"> | |||
<![endif]--> | |||
<meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no"> | |||
|
|||
{% include head-custom.html %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With #31 merged, how would you feel about setting this up this way:
_layouts/default.html
has{% include head-custom.html %}
_includes/head-custom.html
has{% include theme_color_meta_headers.html %}
That way we can move custom headers into head-custom.html
as a single entry point and use this across all the themes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@parkr Great idea! That will also clarify that this is meant to be customized if wanted, and it's certainly a good idea to use this across all themes.
The latest force-push has added changed this accordingly 😄.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: If you prefer underscores as separators, of course I can change that for consistency, just let me know.
I took inspiration from:
https://github.com/mmistakes/so-simple-theme/blob/master/_includes/head-custom.html
which uses the "kebab case" style with dash-separators, but snake-case is also fine with me 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olifre Let's be consistent, whatever we choose.
Either:
head-custom.html
andhead-custom-theme-colors.html
, ORcustom_headers.html
andtheme_color_meta_headers.html
I like the kebab-style system since it's kind of a grouping (we use a head-custom prefix, so head-custom-google-analytics.html
could be added later for example) and it seems easily readable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the kebab-style system since it's kind of a grouping (we use a head-custom prefix, so
head-custom-google-analytics.html
could be added later for example) and it seems easily readable to me.
That's a great point, indeed!
I have pushed another commit doing the renaming, so now all is implemented according to the kebab-style variant 👍 .
This allows users to specify custom head tags, e.g. for a favicon, without patching _default.html. Also migrates the existing theme-color setting include there so it can be overridden easily. closes pages-themes#28
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This allows users to specify custom head tags,
e.g. for a favicon, without patching _default.html.
closes #28