mjisaak / skilling-champion-extension

Skilling Champion extension đŸ±â€đŸ‘€
MIT License
31 stars 8 forks source link

Option to make URLs language-neutral #22

Closed brunolewin closed 3 years ago

brunolewin commented 3 years ago

This adds an option to remove the "en-us" from English URLs to allow non-English users to see the destination page in their preferred language, rather than forcing all to go to the English page. The idea came up in a discussion I had today with French MVPs but should benefit all that share links to a global audience. The PR does includes changes to readme and screenshot in a separate commit.

mjisaak commented 3 years ago

Hi Bruno,

first of all many thanks for the PR. I really like the idea of the language-neutral setting.

I would suggest that we remove whatever language identifier we find within the URL after selecting the neutral option. So not only ...com/en-us but also ...com/de-de ... What do you think?

For the replacement I would then suggest the following regex: (?<=\.com\/)[a-zA-Z]{2}-[a-zA-Z]{2}(?=\/) This will only remove any language identifier after .com/. (I just checked the suitable sites and think that this is always the case)

If you are fine with that, I would add the regex and adopt the readme accordingly

brunolewin commented 3 years ago

Hi Martin, thanks for the quick follow-up. The reason I singled out English is that many sites like e.g. Twitter create “Preview Cards” when sharing URLs. These will be in English if you use a neutral URL, so nothing is lost when dropping the “en-us” but it would alter the experience if you’re writing in another language. I did not want to impose that on users but otherwise, either approach would work great so I leave it at your discretion!

brunolewin commented 3 years ago

On second thought, we should just let people decide what they prefer! I've made a small tweak to allow users to select if they want to modify just English URLs (remove "/en-us") or any language (remove "/xx-xx").

image

I realized after committing that there are a few edge case languages (below) that are supported by docs.microsoft.com that would not be caught in the new regex I used (/(?<=\.com)\/[a-zA-Z]{2}-[a-zA-Z]{2}(?=\/)/i - same as you had suggested, but grabs the /), Something like /(?<=\.com)\/[a-zA-Z]{2}(-[a-zA-Z]{4}){0,1}-[a-zA-Z]{2}(?=\/)/i seems to work, but maybe you could review before we make main.js unreadable? :)

bs-cyrl-ba sr-cyrl-rs bs-latn-ba

mjisaak commented 3 years ago

Hi Bruno,

offering both options should be the best idea. I will have to test the extension, merge the PR, and upload the new version to the marketplaces. Unfortunately, I won't be able to make it this weekend. But I hope at the beginning of next week.

BR and many thanks for your valuable contribution Martin

brunolewin commented 3 years ago

Wonderful - thanks for that!

brunolewin commented 3 years ago

I saw you even published the updated extension already to Chrome store already, thanks a lot for that. Let me share this with some French MVPs and colleagues in the office so they can try out and possibly share more broadly. I'm so glad that we now have such an elegant solution to what has been quite a challenge. THANK YOU

mjisaak commented 3 years ago

Hi Bruno,

I have to thank you. The updated extension is now available in the Chrome and Firefox marketplace. I have also updated the Edge offering but the package is still in review.

Here is the announcement on Twitter: https://twitter.com/martin_jib/status/1407037822478041098

BR Martin