search

lanedirt / OGameX

Open-source OGame redesign clone built with Laravel 11.x.
https://main.ogamex.dev
MIT License
34 stars 15 forks source link

Add colonisation #219

Closed Chewbaka69 closed 3 months ago

lanedirt commented 4 months ago

Thanks for the PR, you're on fire 🔥 !

I just checked the code and have added two comments. Also I tested the change on my local environment and see that overall it works well. I can send a colony ship to an empty planet and it works just like the official game. 👍 However after testing I did find two issues/bugs. Can you take a look at these?

1) When I have a planet with 0 colony ships and go to the galaxy page, I get an error:

Screenshot 2024-06-04 at 22 07 18

2) When I do have a colony ship on my planet the galaxy page works. However the tooltip shows a missing icon:

Screenshot 2024-06-04 at 22 09 09
lanedirt commented 3 months ago

Hi @Chewbaka69, thanks for the fixes.

I tested the PR again locally and have two comments:

1) After checking out this new version I get this error:

Screenshot 2024-06-28 at 09 33 52

I'm not really sure why this only seems to happen on my local environment while the Github tests do run successfully. Anyway, I guess the reason is because the lang() in the layout/main.blade.php calls a straight string translation like so:

<span class="textlabel">@lang('Galaxy')</span>.

And because in this PR you added a new resources/lang/en/galaxy.php file it probably tries to load this translation based on the now existing array file instead of doing a straight string to language translation by doing a lookup in the optional resources/lang/<langcode>.json file.

My request: could you rename the resources/lang/en/galaxy.php to resources/lang/en/t_galaxy.php and change the corresponding translations you added to also use the "_t" prefix as well? Adding this prefix prevents collisions between direct string translations and array style translations.

BTW: the reason I decided to use both translation methods in this project is to not have to define all simple 1 word english strings such as "Galaxy" explicitly in the translation files but instead rely on the optional JSON files for translation lookup.

2) The "no colony ship" error message seems to be reversed. When a planet has > 0 colony ship the error shows up, while it should be the other way around. I added a comment to the code.

Screenshot 2024-06-28 at 09 45 35

Can you take a look at these two comments? Thanks for your help!

Chewbaka69 commented 3 months ago

Thanks for your review and i understand better your choice that look good for me too, so i will fix it asap and push it ;)

lanedirt commented 3 months ago

Looking good, works perfectly now. Will merge now. Thanks again!