sawyerpollard / MineWeather

Chrome Extension that displays a Minecraft scene on New Tabs depending on local weather conditions.
https://chrome.google.com/webstore/detail/mineweather/gklgaaagldobfcacmhhbnbgofohfgneb
67 stars 11 forks source link

Localization #12

Closed GreatSymphonia closed 3 years ago

GreatSymphonia commented 3 years ago

Implementing localization by making references to a JSON file with translations.

I considered the idea to implementing with something like this But, due to the small scale of this project, it would take a lot of ressources to implement for little advantages in the end.

This is a draft I made with all my lack of knowledge of javascript, it works and I would consider it a proof of concept more than a final release.

I'll fix all the issues you'll point out to me (when I'll get enough time).

GreatSymphonia commented 3 years ago

This PR is in answer to this issue: #7

rhld16 commented 3 years ago

While you're general implementation looks great, I'd just like to note is it really necessary to add the 30kb jQuery library for a function that could be added in ~10 lines. I know 30kb isn't a lot but when looking at how little of the library is being utilized it may not be entirely worth it.

GreatSymphonia commented 3 years ago

Yeah, I understand your point, this comes from my lack of understanding of JS in general, I don’t know how I could implement this function from JQuery.

I’ll try to find something though. Thanks for the insight!

rhld16 commented 3 years ago

Checkout my PR on your PR (git can be confusing lol) which is a really simple fix for it.

sawyerpollard commented 3 years ago

Thank you for this pull request! I'm going to check them out this weekend.

GreatSymphonia commented 3 years ago

@rhld16, I accepted your PR on my fork, thanks for the fix! Now we can implement all without jQuery.

GreatSymphonia commented 3 years ago

@randy-halim corrected

sawyerpollard commented 3 years ago

First off, thanks so much for your work on this feature! I really like your approach, but I ended up implementing localization with the Chrome i18n API. I also created a new branch called "community" and if you make revisions to your implementation I'd be happy to add it to that branch.

I'll note a couple of parts of the code that can be tweaked.

The French translations look great! The way that Chrome does internationalization is with "_locales" folder. So, for French, you'd make a folder in "_locales" called "fr" and copy the "messages.json" file from the "en" folder. Then the translations go there. There's a lot more info on: https://developer.chrome.com/docs/extensions/reference/i18n/

GreatSymphonia commented 3 years ago

First off, thanks so much for your work on this feature! I really like your approach, but I ended up implementing localization with the Chrome i18n API. I also created a new branch called "community" and if you make revisions to your implementation I'd be happy to add it to that branch.

I'll note a couple of parts of the code that can be tweaked.

  • The switch statement can be replaced with const json = loc[language];.
  • weatherElement.innerHTML = `${Math.round(temperature)}\u00B0`+json.and+`${description}`; can instead be written as weatherElement.innerHTML = `${Math.round(temperature)}\u00B0 ${json.and} ${description}`; No need to use back ticks and addition.
  • I wouldn't create separate French and English buttons. This should ideally be done dynamically—maybe use a dropdown menu?

The French translations look great! The way that Chrome does internationalization is with "_locales" folder. So, for French, you'd make a folder in "_locales" called "fr" and copy the "messages.json" file from the "en" folder. Then the translations go there. There's a lot more info on: https://developer.chrome.com/docs/extensions/reference/i18n/

Thanks a lot for all that! I'll look more into it when I'll get a bit more time (midterms are coming quick ^^)

GreatSymphonia commented 3 years ago

will reimplement in the now used format