Skip to content

Conversation

@sejas
Copy link
Member

@sejas sejas commented May 12, 2025

Related issues

Proposed Changes

  • Unify the getBlogLink and translateLink functions that translate links into a single, easy-to-use function called getLocalizedLink, maintain a dictionary for centralized link usage, and preserve the essence suggested in Translate what's new links #1368 (comment)

Testing Instructions

  • Run npm start
  • Confirm the links in what's new are still working. You can follow the tests on Translate what's new links #1368
  • Confirm the links pointing to the docs are still working in English, and Spanish.
    • Sections: 'system menu', 'content-tab-import-export', 'site-form', 'sync-connected-sites', and 'top-bar'

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@sejas sejas self-assigned this May 12, 2025
@sejas sejas requested a review from a team May 12, 2025 14:35
@sejas sejas mentioned this pull request May 12, 2025
1 task
Copy link
Contributor

@gcsecsey gcsecsey left a 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 ];
Copy link
Contributor

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.

Copy link
Member Author

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.

@sejas
Copy link
Member Author

sejas commented May 12, 2025

@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 getLink can be used to generate docs and blog links, and potentially any kind of links.

Let me know what you think.

@sejas sejas requested a review from wojtekn May 12, 2025 17:19
Comment on lines 30 to 33
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/',
Copy link
Member Author

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.

Comment on lines 42 to 47
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 >;
Copy link
Member Author

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.

Copy link
Contributor

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: {
		[...]
	},
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer having everything in the same object. I'm not sure if forcing a prefix is useful, but I see that it can help with decision-making when seeing the autocomplete while calling the function.

Screenshot 2025-05-13 at 09 49 07

I'll leave it as it is for now. We can always merge the objects and remove the prefix check later.

Copy link
Contributor

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.

Copy link
Contributor

@fredrikekelund fredrikekelund left a 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.

Copy link
Contributor

@wojtekn wojtekn left a 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!

Comment on lines 42 to 47
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 >;
Copy link
Contributor

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: {
		[...]
	},
}

Copy link
Contributor

@nightnei nightnei left a 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

@sejas sejas merged commit c28ce35 into trunk May 13, 2025
8 of 9 checks passed
@sejas sejas deleted the remove/STU-470-refactor-get-docs-link-to-translate-link branch May 13, 2025 10:55
@sejas sejas changed the title Refactor getDocsLink in favor of translateLink Refactor getDocsLink in favor of getLocalizedLink May 13, 2025
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.

6 participants