Closed edisontim closed 3 months ago
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 | Jun 28, 2024 0:55am |
โฑ๏ธ Estimated effort to review [1-5] | 4 |
๐งช Relevant tests | No |
๐ Security concerns | No |
โก Key issues to review |
Possible Bug: The newly exported functions checkIfArmyLostAFinishedBattle and checkIfArmyAlive in useArmies.tsx are exported but it's unclear if their internal logic has been verified against the new contexts they will be used in. It's crucial to ensure these functions perform accurately in all scenarios where they are now accessible. |
Code Clarity: In StructureCard.tsx , the refactoring for simplifying nested conditions and removing redundant code is good. However, it's important to ensure that the logic for displaying components based on showMergeTroopsPopup and other conditions remains correct and that no edge cases are missed. | |
UI Consistency: Changes in BattleActions.tsx introduce new conditions for enabling battle actions. These need thorough testing to ensure they behave as expected under various game states. |
Category | Suggestion | Score |
Best practice |
Improve type safety by adding specific type annotations to function parameters___ **Consider adding type annotations for the parametersBattle , Army , and army in the function checkIfArmyLostAFinishedBattle to improve type safety and code readability.**
[client/src/hooks/helpers/useArmies.tsx [572]](https://github.com/BibliothecaDAO/eternum/pull/1039/files#diff-6d68c14fbe3262def9a16f121115d2006c7c34bf942b3e5d8ed7c076d2b89027R572-R572)
```diff
-export const checkIfArmyLostAFinishedBattle = (Battle: any, Army: any, army: any) => {
+export const checkIfArmyLostAFinishedBattle = (Battle: BattleType, Army: ArmyType, army: ArmyInfo) => {
```
Suggestion importance[1-10]: 8Why: Adding specific type annotations improves type safety and code readability, which is a best practice in TypeScript. This suggestion is directly relevant to the new code introduced in the PR. | 8 |
Enhance type safety by using specific types instead of
___
**Replace the | 8 | |
Possible bug |
Correct the concatenation in the
___
**Ensure that the | 8 |
Maintainability |
Simplify conditional rendering in JSX by consolidating repeated condition checks___ **Replace the repeated checks forshowMergeTroopsPopup with a single conditional rendering block to simplify the JSX structure and improve readability.** [client/src/ui/components/hyperstructures/StructureCard.tsx [74-90]](https://github.com/BibliothecaDAO/eternum/pull/1039/files#diff-dc87a3d6de7675f4a22a24979bc532796ebef68a9ac4cc8ed2d64217038fecddR74-R90) ```diff -{!showMergeTroopsPopup && formattedRealmAtPosition && ( -
{ownArmySelected && (
+) : (
+ <>
+ {formattedRealmAtPosition ? (
+ Suggestion importance[1-10]: 7Why: This suggestion improves the maintainability and readability of the JSX code by consolidating repeated condition checks, making the code cleaner and easier to understand. | 7 |
Simplify conditional text rendering in the
___
**Refactor the | 3 | |
Possible issue |
Ensure the grid layout is correctly applied to maintain the layout structure___ **Add agrid-cols-3 class to the div element to ensure that the child components are evenly distributed in a three-column layout, which was omitted in the PR.** [client/src/ui/modules/military/battle-view/Troops.tsx [6]](https://github.com/BibliothecaDAO/eternum/pull/1039/files#diff-91a2d2f8f54e7a7655eb1a9a90930dd08eee7b9fa3063d3a5847fa77486da641R6-R6) ```diff -
+
```
Suggestion importance[1-10]: 7Why: Removing the redundant "flex" class ensures that the grid layout is correctly applied, which is important for maintaining the intended layout structure. | 7 |
Enhancement |
Change flex direction to column for better alignment and readability of resource items___ **Replace theflex-row with flex-col in the className to maintain vertical alignment of elements, which is more suitable for displaying resource lists and improves readability.** [client/src/ui/modules/military/battle-view/LockedResources.tsx [18]](https://github.com/BibliothecaDAO/eternum/pull/1039/files#diff-359d4a5ec2ea695cafb43989d9412232fc58a85aae799cdc84e83e4b128d9f42R18-R18) ```diff -
+
```
Suggestion importance[1-10]: 5Why: Changing the flex direction to column could improve readability, but it depends on the overall design and layout requirements. This is a minor enhancement. | 5 |
Add a responsive width property to the avatar's class for consistent sizing___ **Consider adding awidth property to the className attribute to ensure the avatar maintains a consistent width across different screen sizes or layout changes.** [client/src/ui/modules/military/battle-view/EntityAvatar.tsx [40]](https://github.com/BibliothecaDAO/eternum/pull/1039/files#diff-502ca5a86df525ba71ab3ea0e5b8c19ade9c3e3a8611c3c07426b22cbdb83f4dR40-R40) ```diff -className="h-44 w-44 clip-angled rounded-full object-cover border-gold/50 border-4 -mt-12 bg-black" +className="h-44 w-44 clip-angled rounded-full object-cover border-gold/50 border-4 -mt-12 bg-black w-[10vw]" ``` Suggestion importance[1-10]: 3Why: The suggestion adds a responsive width property, but the existing code already has a fixed width of "w-44". This change is minor and may not be necessary. | 3 |
PR Type
Enhancement, Bug fix
Description
checkIfArmyLostAFinishedBattle
andcheckIfArmyAlive
inuseArmies.tsx
.InventoryResources
andRealmListItem
.Changes walkthrough ๐
16 files
useArmies.tsx
Export utility functions for army status checks
client/src/hooks/helpers/useArmies.tsx
checkIfArmyLostAFinishedBattle
andcheckIfArmyAlive
functions.
StructureCard.tsx
Refactor StructureCard component and adjust styles
client/src/ui/components/hyperstructures/StructureCard.tsx
Battle.tsx
Enhance Battle component layout and imports
client/src/ui/components/military/Battle.tsx
BUILDING_IMAGES_PATH
import.InventoryResources.tsx
Add toggle functionality to InventoryResources component
client/src/ui/components/resources/InventoryResources.tsx
setShowAll
prop to toggle visibility of all resources.Army.tsx
Pass structure ownership status to CombatLabel
client/src/ui/components/worldmap/armies/Army.tsx - Passed `structureIsMine` prop to `CombatLabel`.
CombatLabel.tsx
Update CombatLabel to reflect structure ownership
client/src/ui/components/worldmap/armies/CombatLabel.tsx
structureIsMine
prop toCombatLabel
.structureIsMine
prop.HexagonInformationPanel.tsx
Adjust HexagonInformationPanel positioning
client/src/ui/components/worldmap/hexagon/HexagonInformationPanel.tsx - Adjusted CSS classes for better positioning.
RealmListItem.tsx
Add resource visibility toggle to RealmListItem
client/src/ui/components/worldmap/realms/RealmListItem.tsx
Battle.tsx
Enhance Battle component layout
client/src/ui/modules/military/battle-view/Battle.tsx - Adjusted CSS classes for better layout.
BattleActions.tsx
Refactor BattleActions component and add utility functions
client/src/ui/modules/military/battle-view/BattleActions.tsx
isAttackable
andisClaimable
conditions.handleBattleStart
andhandleBattleClaim
functions.BattleDetails.tsx
Enhance BattleDetails component layout
client/src/ui/modules/military/battle-view/BattleDetails.tsx - Adjusted CSS classes for better layout.
BattleProgressBar.tsx
Update BattleProgressBar logic and layout
client/src/ui/modules/military/battle-view/BattleProgressBar.tsx
BattleView.tsx
Enhance BattleView component layout
client/src/ui/modules/military/battle-view/BattleView.tsx - Adjusted CSS classes for better layout.
EntityAvatar.tsx
Enhance EntityAvatar component layout
client/src/ui/modules/military/battle-view/EntityAvatar.tsx - Adjusted CSS classes for better layout.
LockedResources.tsx
Enhance LockedResources component layout
client/src/ui/modules/military/battle-view/LockedResources.tsx - Adjusted CSS classes for better layout.
Troops.tsx
Enhance Troops component layout
client/src/ui/modules/military/battle-view/Troops.tsx - Adjusted CSS classes for better layout.