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

DOP-4457: Strip away locale code from slugs before localizing them #1041

Merged
merged 3 commits into from
Mar 14, 2024

Conversation

rayangler
Copy link
Collaborator

@rayangler rayangler commented Mar 14, 2024

Stories/Links:

DOP-4457

Current Behavior:

Server prod

Staging Links:

Server staging - mostly to spot check that things (the version dropdown, hreflang links, and footer language selector) aren't broken. Unfortunately, this will need to be tested in production due to Smartling's production-only logic.

Notes:

The situation is that Smartling overrides some frontend logic on their end to allow the frontend to properly link to other docs pages within the same locale. We purposely have logic that localizes URLs ourselves, so that we can link to them (for example: the language selector at the footer). This works fine on the English docs, but on the translated docs, Smartling's overridden localized slugs cause our localization of the same slug to result in two locale codes in the URL. A solution would be to remove the locale code from the beginning of a slug (if it exists), before localizing the slug.

README updates

    • This PR introduces changes that should be reflected in the README, and I have made those updates.
    • This PR does not introduce changes that should be reflected in the README

@rayangler rayangler requested a review from seungpark March 14, 2024 19:04
@rayangler rayangler marked this pull request as ready for review March 14, 2024 19:08
@rayangler rayangler requested a review from mmeigs March 14, 2024 19:21
const smartlingNoRewriteClass = 'sl_norewrite';
const smartlingNoRewriteClass = 'sl_opaque';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious: what Smartling behavior changes with this?

Copy link
Collaborator Author

@rayangler rayangler Mar 14, 2024

Choose a reason for hiding this comment

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

Great question! This was the solution provided to me by Arnaud (from Smartling) to prevent the hreflang link from being overwritten. For some reason the sl_norewrite class wasn't doing it so he wanted me to try it the opaque class. Here's a link to the docs, although I'm not sure how different they'd be for this use case.

I should've added this to the description, but I deployed one versions of the Node docs, and you can see in Simplified Chinese docs that the hreflang link for x-default is incorrect (it should be the English URL), most likely because it's being overwritten.

const code = localeCode && validateCode(localeCode) ? localeCode : getCurrLocale();
const languagePrefix = code === 'en-us' ? '' : `${code}/`;
let newPath = languagePrefix + pathname;
let newPath = languagePrefix + unlocalizedPath;
if (pathname.startsWith('/')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this conditional use the unlocalizedPath or does it matter because it looks like you've tried to replicate the leading character anyway?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe it should be the pathname to ensure the "original" slug is the source of truth. But I don't think it matters? 🤔

Copy link
Collaborator

@seungpark seungpark left a comment

Choose a reason for hiding this comment

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

discussed offline weird behavior by Smartling team to overwrite the slug property (written in build time). will have to keep tabs on their changes to see if they submit another fix...


// Replace from the original slug to maintain original form
const res = validateCode(firstPathSlug) ? normalizePath(slug.replace(firstPathSlug, '')) : slug;
if (res.startsWith('/') && !slug.startsWith('/')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[not blocking] seems like we are doing a lot of slash handling. would prefer if we could handle it in the exported function and helpers can just do simple string manipulations on input. also not seeing where this is called so may not have to export

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed that there's a lot of slash handling... I've made the function local, but as discussed offline, I'm afraid of how Smartling is handling these slugs, so I want to ensure that the output string doesn't accidentally manipulate the input. Happy to revisit this logic during a less stressful situation to help clean up

@rayangler rayangler merged commit 1576426 into main Mar 14, 2024
4 checks passed
@rayangler rayangler deleted the DOP-4457 branch March 14, 2024 21:05
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.

3 participants