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

feat: additional catchall hreflang tags #597

Merged
merged 16 commits into from
Feb 24, 2020
Merged

Conversation

manniL
Copy link
Contributor

@manniL manniL commented Feb 16, 2020

Resolves #522

@manniL manniL requested a review from rchl February 16, 2020 22:13
@codecov
Copy link

codecov bot commented Feb 16, 2020

Codecov Report

Merging #597 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #597   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines         121    121           
  Branches       31     31           
=====================================
  Hits          121    121

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78812db...ab632f4. Read the comment docs.

@rchl
Copy link
Collaborator

rchl commented Feb 17, 2020

I thought the idea was to add that more-generic meta tag in addition to original one, rather than replace it.

Also, I would like this to be more automatic. I was thinking to include the list of all locales in the module and derive one automatically (from first locale-country code seen, for example). Of course, it would still be possible to override that with an extra property.

As for the property name, maybe call it something closer to documentation - for example isCatchallLocale.

And the new property should be documented in seo.md and options-reference.md.

@manniL
Copy link
Contributor Author

manniL commented Feb 18, 2020

I thought the idea was to add that more-generic meta tag in addition to original one, rather than replace it.

I don't think it's intended to use the same link/url for a specific language and a catch-all. At least I can't find an example (not in the guidelines nor on 3rd party SEO-related pages like Moz, yoast, ...) reflecting that.

Please point out if I'm misunderstanding that or when there are other examples ☺️

UPDATE: As shown in this article it is reasonable to do so. will work on an implementation

Also, I would like this to be more automatic. I was thinking to include the list of all locales in the module and derive one automatically (from first locale-country code seen, for example). Of course, it would still be possible to override that with an extra property.

Can't be automated easily in case the first point is valid.

Makes sense!

As for the property name, maybe call it something closer to documentation - for example isCatchallLocale.

Will change ☺️

And the new property should be documented in seo.md and options-reference.md.

True!

@manniL manniL self-assigned this Feb 18, 2020
@rchl
Copy link
Collaborator

rchl commented Feb 18, 2020

I don't think it's intended to use the same link/url for a specific language and a catch-all. At least I can't find an example (not in the guidelines nor on 3rd party SEO-related pages like Moz, yoast, ...) reflecting that.

How about that part (see emphasis)?

If you have several alternate URLs targeted at users with the same language but in different locales, it's a good idea also to provide a catchall URL for geographically unspecified users of that language. For example, you may have specific URLs for English speakers in Ireland (en-ie), Canada (en-ca), and Australia (en-au), but should also provide a generic English (en) page for searchers in, say, the US, UK, and all other English-speaking locations. It can be one of the specific pages, if you choose.
https://support.google.com/webmasters/answer/189077

@manniL
Copy link
Contributor Author

manniL commented Feb 18, 2020

@rchl Yup, you are right! Already correct my psot above after seeing another confirming tweet from John Mueller ☺️

@manniL manniL changed the title feat: region-independent hreflang tags feat: additional catchall hreflang tags Feb 18, 2020
test/module.test.js Outdated Show resolved Hide resolved
src/templates/seo-head.js Outdated Show resolved Hide resolved
@manniL manniL requested a review from rchl February 24, 2020 13:01
@rchl
Copy link
Collaborator

rchl commented Feb 24, 2020

We should also have test that uses IsCatchallLocale.

Also a friendly reminder about documentation. :)

src/templates/seo-head.js Show resolved Hide resolved
test/module.test.js Outdated Show resolved Hide resolved
test/module.test.js Outdated Show resolved Hide resolved
@manniL
Copy link
Contributor Author

manniL commented Feb 24, 2020

We should also have test that uses IsCatchallLocale.

Covered in a separate fixture! ☺️

Also a friendly reminder about documentation. :)

Done now as well, thanks for the reminder 👍🏻

@manniL manniL requested a review from rchl February 24, 2020 15:57
docs/seo.md Outdated Show resolved Hide resolved
docs/seo.md Outdated Show resolved Hide resolved
test/module.test.js Outdated Show resolved Hide resolved
@rchl rchl merged commit ebd2213 into master Feb 24, 2020
@rchl rchl deleted the feat/region-indepentent-lang branch February 24, 2020 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SEO: missing region-independant link for language
2 participants