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

fix: using strict equality in generateLinkRelAlternateProps method in… #100

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Darter90
Copy link

if I understood correctly @FaberVitale, you make a for loop on the back-end side to generate the links in the header, so you only have to add the parameters in i18nLang and routing.ts files; so you gave an order on both sides (first: it, second: en).

But the for loop that must generate the links, acts only on hreflang.
So what happens... lang does not switch its value, while hreflang does... and since you say that the two values ​​must not be strictly equal, the code generates incorrect links by inverting the values ​​of "it" and "en".

So the fix, without having to do a total refactoring of the method (which perhaps could also be done) is simply to change the operator with strictly equality.

I'm posting pictures with some tests in localhost after the fix:

eventi-passati/1/
image

past-events/1/
image

prossimi-eventi/
image

upcoming-events
image

Copy link

netlify bot commented Jan 28, 2025

Deploy Preview for romajs ready!

Name Link
🔨 Latest commit 3e17d9d
🔍 Latest deploy log https://app.netlify.com/sites/romajs/deploys/6798fd43e0e361000862970c
😎 Deploy Preview https://deploy-preview-100--romajs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@FaberVitale
Copy link
Collaborator

FaberVitale commented Jan 29, 2025

Hi @Darter90,
Thanks for your contribution.

The goal of that function is to automatically generate the alternate links.

So if, for instance if the url is /it/my-slug and we support ['it', 'es', 'en'] the logic will add to the page

<link rel="alternate" href="/es/my-slug" hreflang="es">
<link rel="alternate" href="/esnmy-slug" hreflang="en">

This is important because when a user lands on a page in a lang they do not speak,
the browser will prompt them to navigate to another page.

The logic in generateLinkRelAlternateProps could be summerized as:

  1. Test input Pathname => if it is index page return otherwise go to next step
  2. Try to extract the lang path segment from the pathname (see routeParamsRegex regex).
  3. If we can't find the lang pathname return immediately otherwise go to next step
  4. For each lang that is not the current one we generate the alternate link.

I do not think we want to implement the change suggested in this PR.
We want to generate the alternate links, those links must not have the current locale.

Overall the current implementation I wrote of generateLinkRelAlternateProps is a bit naive and should probably be implemented differently.
See #90 for further info.

Any help with #90 or other issues would be appreciated.
Thanks 🙏

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.

2 participants