-
Notifications
You must be signed in to change notification settings - Fork 53
Refactor getDocsLink in favor of getLocalizedLink #1375
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
Refactor getDocsLink in favor of getLocalizedLink #1375
Conversation
gcsecsey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this follow-up @sejas! 🙌
The refactor looks good to me and works well. I can still open the Spanish blog post from the What's New section using this branch.
CleanShot.2025-05-12.at.15.41.05.mp4
| ] ); | ||
|
|
||
| export function translateLink( locale: SupportedLocale, url: string ): string { | ||
| const translation = URL_TRANSLATIONS.get( url )?.[ locale ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the single responsibility principle, and considering that the two implementations differ significantly, I think it would make sense to have two separate files, one for getDocsLink and another for getBlogLink. Alternatively, have getDocsLink which dynamically changes the language, and another one, e.g. getLink that uses the mapping array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. So I opted to share the same logic and architecture. Now the translatable links use a dict/object. Types force to have the english link as a type requirement. We only have a unique function to "translate" links, which I think is important for consistency.
|
@wojtekn , thanks for the feedback. I really appreciate it. Now the code it's cleaner. We are forced to add all the links into the translation array. The sam function Let me know what you think. |
| en: 'https://wordpress.com/blog/2025/03/17/studio-wordpress-php-versions/', | ||
| es: 'https://wordpress.com/es/blog/2025/04/02/modifica-las-versiones-de-wordpress-y-php-de-tu-sitio-local-con-studio/', | ||
| fr: 'https://wordpress.com/fr/blog/2025/03/28/studio-wordpress-php-versions/', | ||
| 'pt-br': 'https://wordpress.com/pt-br/blog/2025/04/02/estudio-wordpress-php-versoes/', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the full links makes it easier to test whether they work or return a 404.
| blogCustomDomainsHttps: { | ||
| en: 'https://wordpress.com/blog/2025/03/31/studio-custom-domains-https/', | ||
| es: 'https://wordpress.com/es/blog/2025/03/31/studio-custom-domains-https/', | ||
| 'pt-br': 'https://wordpress.com/pt-br/blog/2025/04/03/estudio-dominios-personalizados-https/', | ||
| }, | ||
| } satisfies Record< `blog${ string }`, TranslatedLink >; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These types force the link keys to start with blog and docs, respectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last nit on method signature - if we want to enforce a prefix, would it be clearer to change the method signature and add another parameter for that?
getLink( 'en', 'docs', 'importExport' )
Alternatively, what if we skipped enforcing that, and used one array for both docs and blog links, making it generic and not tied to the resource type it stores?
const LINKS = {
blogPhpVersions: {
[...]
},
docsPhpVersions: {
[...]
},
dotOrgBlogGutenber: {
[...]
},
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks for sharing this use case.
fredrikekelund
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement 👍 Left some minor suggestions.
wojtekn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for simplifying that!
| blogCustomDomainsHttps: { | ||
| en: 'https://wordpress.com/blog/2025/03/31/studio-custom-domains-https/', | ||
| es: 'https://wordpress.com/es/blog/2025/03/31/studio-custom-domains-https/', | ||
| 'pt-br': 'https://wordpress.com/pt-br/blog/2025/04/03/estudio-dominios-personalizados-https/', | ||
| }, | ||
| } satisfies Record< `blog${ string }`, TranslatedLink >; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last nit on method signature - if we want to enforce a prefix, would it be clearer to change the method signature and add another parameter for that?
getLink( 'en', 'docs', 'importExport' )
Alternatively, what if we skipped enforcing that, and used one array for both docs and blog links, making it generic and not tied to the resource type it stores?
const LINKS = {
blogPhpVersions: {
[...]
},
docsPhpVersions: {
[...]
},
dotOrgBlogGutenber: {
[...]
},
}
nightnei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and works well 👍
TIL satisfies

Related issues
Proposed Changes
getLocalizedLink, maintain a dictionary for centralized link usage, and preserve the essence suggested in Translate what's new links #1368 (comment)Testing Instructions
npm startPre-merge Checklist