Skip to content
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

Fix/donkey img #1717

Merged
merged 3 commits into from
Sep 25, 2024
Merged

Fix/donkey img #1717

merged 3 commits into from
Sep 25, 2024

Conversation

bob0005
Copy link
Collaborator

@bob0005 bob0005 commented Sep 25, 2024

Fixes #1710
Fixes #1627

Copy link

vercel bot commented Sep 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
eternum ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2024 5:49pm

Copy link

Preparing PR description...

Copy link

Preparing review...

Copy link
Contributor

@mentatbot mentatbot bot left a comment

Choose a reason for hiding this comment

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

The changes in this PR appear to address the issues mentioned in the summary (#1710 and #1627). The modifications improve consistency in naming (changing 'Donkeys' to 'Donkey') and add a helpful null check in the ArmyManager. However, there are a few areas that could be further improved:

  1. The description for the Donkey resource is very brief and could be expanded.
  2. The image URL for the Donkey resource still points to a wheat image and should be updated.
  3. It might be beneficial to review the codebase for other opportunities to add null checks or use enums consistently.

Overall, these changes seem to be a step in the right direction, but a bit more attention to detail could make this PR even better.

Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!

client/src/three/components/ArmyManager.ts Show resolved Hide resolved
client/src/ui/elements/ResourceIcon.tsx Show resolved Hide resolved
value: 249,
colour: "#ec4899",
id: 249,
description: "Donkeys.",
description: "Donkey.",
img: "https://github.com/BibliothecaForAdventurers/voxel-resources/blob/main/compressed/wheat.gif?raw=true",
Copy link
Contributor

Choose a reason for hiding this comment

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

The image URL for the Donkey resource still points to a wheat image. Update this to an appropriate donkey image if one is available.

Suggested change
img: "https://github.com/BibliothecaForAdventurers/voxel-resources/blob/main/compressed/wheat.gif?raw=true",
img: "https://github.com/BibliothecaForAdventurers/voxel-resources/blob/main/compressed/donkey.gif?raw=true",

@edisontim edisontim merged commit 268a7f8 into main Sep 25, 2024
8 checks passed
@edisontim edisontim deleted the fix/donkey-img branch September 25, 2024 18:12
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.

[Client] Recurring error in dev console [client] donkey icon not displaying in liquidity pools
2 participants