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.

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 og:locale support #166

Merged
merged 13 commits into from
Apr 4, 2017
Merged

Add og:locale support #166

merged 13 commits into from
Apr 4, 2017

Conversation

aarongustafson
Copy link
Contributor

No description provided.

@aarongustafson
Copy link
Contributor Author

Addresses #167

@benbalter
Copy link
Collaborator

Is there a pattern for what the key should be called? Can we use site.lang (and post.lang) to share a setting with jekyll-feed?

@aarongustafson
Copy link
Contributor Author

@benbalter It is not a standard config in Jekyll (it was in Octopress), but I’m absolutely game to piggyback on jekyll-feed. PR update forthcoming.

@aarongustafson
Copy link
Contributor Author

@benbalter Made the changes & merged back your master branch.

@@ -7,4 +7,5 @@
/pkg/
/spec/reports/
/tmp/
/bin/
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following the RSpec install recommendation which created that directory. I didn’t want to bundle it with the repo.

README.md Outdated
The SEO tag will respect the following YAML front matter if included in a post, page, or document:

* `title` - The title of the post, page, or document
* `description` - A short description of the page's content
* `image` - URL to an image associated with the post, page, or document (e.g., `/assets/page-pic.jpg`)
* `author` - Page-, post-, or document-specific author information (see below)
* `locale` - Page-, post-, or document-specific language information
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will need to be lang. We should also indicate that this can be site.lang.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Fix coming. Sorry I missed that.

README.md Outdated
The SEO tag will respect the following YAML front matter if included in a post, page, or document:

* `title` - The title of the post, page, or document
* `description` - A short description of the page's content
* `image` - URL to an image associated with the post, page, or document (e.g., `/assets/page-pic.jpg`)
* `author` - Page-, post-, or document-specific author information (see below)
* `locale` - Page-, post-, or document-specific language information
Copy link
Member

Choose a reason for hiding this comment

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

This should be changed to lang, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep.

@@ -97,6 +97,8 @@
{% assign seo_page_image = seo_page_image | escape %}
{% endif %}

{% assign seo_page_locale = page.lang | default: site.lang | default: "en_US"" %}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we need to default to "en_US" instead of just not setting anything if the user didn't ask for it?

Copy link
Member

Choose a reason for hiding this comment

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

Can we instead call this seo_page_lang for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we can.

The OG docs default to “en_US”. I thought it made sense to be explicit. If you feel otherwise, we can drop the tag without a defined language (or even if it is “en_US”).

Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Thanks for all the work you put into this 👍🍻

@@ -97,6 +97,8 @@
{% assign seo_page_image = seo_page_image | escape %}
{% endif %}

{% assign seo_page_lang = page.lang | default: site.lang | default: "en_US" %}
Copy link
Member

Choose a reason for hiding this comment

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

I don't know about default values here, but I suppose I don't have a strong preference either way.

Choose a reason for hiding this comment

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

Should there be some sort of string normalization applied with maybe a | replace: '-', '_'?

For example it's possible to have a site.lang of en-US for the lang attribute in <html lang="...">. Same for xml:lang in an Atom feed <feed xmlns="http://www.w3.org/2005/Atom" xml:lang="...">.

In both cases it appears they expect values of language-REGION, e.g. en-US, where as og:locale wants en_US with an underscore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I can make that happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And done (assuming I didn’t do anything to annoy Rubocop).

@benbalter
Copy link
Collaborator

Nice work @aarongustafson! 🌐 🎏 🈂️

@benbalter benbalter merged commit dcf646c into jekyll:master Apr 4, 2017
@aarongustafson
Copy link
Contributor Author

Yay! Any idea when the next release will be?

@aarongustafson aarongustafson deleted the add-locale-support branch April 4, 2017 14:53
@benbalter
Copy link
Collaborator

If #151 lands soon, it'd be a solid release to get that in.

@jekyll jekyll locked and limited conversation to collaborators Apr 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants