search

gatsbyjs / themes

This is a repo for Gatsby's official themes.
138 stars 77 forks source link

fix: fix hrefLang x-default tag #125

Closed IgorZanellaDev closed 3 years ago

IgorZanellaDev commented 3 years ago

Hi, there is a SEO problem in the x-default link tag. As you can see here x-default is referred to defaultSiteUrl without path, so hrefLang will be wrong in the pages which are not the home page.

I modified the href to use the default language current page.

If for someone x-default like this is good like this, we can also add a prop to choose whether use defaultSiteUrl or current page.

Wrong for me ⛔️

<link data-react-helmet="true" rel="alternate" hrefLang="x-default" href="http://localhost:8000/"/>
<link data-react-helmet="true" rel="alternate" hrefLang="en-US" href="http://localhost:8000/features/crm/"/>
<link data-react-helmet="true" rel="alternate" hrefLang="it-IT" href="http://localhost:8000/it/features/crm/"/>

Right for me ✅

<link data-react-helmet="true" rel="alternate" hrefLang="x-default" href="http://localhost:8000/features/crm/"/>
<link data-react-helmet="true" rel="alternate" hrefLang="en-US" href="http://localhost:8000/features/crm/"/>
<link data-react-helmet="true" rel="alternate" hrefLang="it-IT" href="http://localhost:8000/it/features/crm/"/>
olivier-lacroix commented 3 years ago

Hi @IgorZanellaDev , just noticed this as well.

I believe you also need to take into account the prefixDefault parameter to prepend the path with defaultLang ?

IgorZanellaDev commented 3 years ago

Yes, you are right, I only copypasted.

IgorZanellaDev commented 3 years ago

Done, I added defaultPrefix check, hope someone can see this PR.

olivier-lacroix commented 3 years ago

Thx @IgorZanellaDev. Yup, that’d be cool!

emdotem commented 3 years ago

@sidharthachatterjee @pieh can you review and eventually merge it?

It's something we miss in our landing as well.

LekoArts commented 3 years ago

I've chosen the homepage on purpose due to recommendations from Google: https://developers.google.com/search/docs/advanced/crawling/localized-versions#xdefault

If we want to introduce this feature I'd want it as a prop on the SEO component and keep the existing behavior for now by setting the default value false then.

IgorZanellaDev commented 3 years ago

Ok, I added a boolean in gatsby-config options, could you check now? I also updated the readme file. The boolean is in useLocalization hook.

LekoArts commented 3 years ago

Hi!

Sorry if I was unclear on this but I meant that it should be a prop on the <SEO /> component, e.g. <SEO xDefaultCurrent /> and then it would have the functionality. In the SEO component itself it would have xDefaultCurrent = false as a parameter.

IgorZanellaDev commented 3 years ago

Ok, but how can I pass the prop to seo component? In plugin config like I did?

LekoArts commented 3 years ago

react-helmet dedupes itself so if you place the <SEO /> component (imported from the theme) in your page and add the prop it should override the behavior from the theme. You could also shadow (https://www.gatsbyjs.com/docs/how-to/plugins-and-themes/shadowing/) the SEO component and add the prop there.

The first assumption I'd need to double check first though.

IgorZanellaDev commented 3 years ago

After some tests I decided to shadow the component in our project, I close the PR, thank you, bye.