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 | Jul 4, 2024 10:46am |
β±οΈ Estimated effort to review: 3 π΅π΅π΅βͺβͺ |
π§ͺ No relevant tests |
π No security concerns identified |
β‘ Key issues to review **Possible Bug:** In `InventoryResources.tsx`, the logic for displaying resources seems to be modified to include dynamic resources. However, the slicing logic for `dynamicResources` and `inventoryResources` might not correctly handle cases where the sum of both exceeds the maximum allowed (`max`). This could lead to incorrect rendering of resources or an incorrect number of resources being displayed. **Code Clarity:** In `StructureListItem.tsx`, the conditional rendering logic inside the JSX is quite complex and could benefit from being broken down into smaller, more manageable components or functions. This would improve readability and maintainability. |
Category | Suggestion | Score |
Maintainability |
Refactor the rendering of
___
**Extract the JSX logic for rendering | 8 |
Refactor complex conditional rendering into a separate function___ **Use a ternary operator or a separate rendering function to handle the conditionalrendering logic for FragmentMine and Hyperstructure , to simplify the JSX structure and improve readability.** [client/src/ui/components/worldmap/structures/StructureListItem.tsx [37-48]](https://github.com/BibliothecaDAO/eternum/pull/1069/files#diff-76d2d7706286ec37ce504f8bfde4367b70141a870a017e824e194ef5b0facc26R37-R48) ```diff -{String(structure.category) === "FragmentMine" ? ( -
-
- ) : (
- {HYPERSTRUCTURE_POINTS_PER_CYCLE} points/tick
- )}
+{renderStructureDetails(structure)}
```
Suggestion importance[1-10]: 8Why: Simplifying the JSX structure by moving conditional rendering to a separate function improves readability and maintainability. | 8 | |
Replace inline string manipulation with a utility function for formatting___ **Consider using a utility function to format thestructure.category for display purposes instead of inline string manipulation. This will improve code readability and maintainability.** [client/src/hooks/helpers/useStructures.tsx [102-104]](https://github.com/BibliothecaDAO/eternum/pull/1069/files#diff-396eaa9679c616f55b4078861124f09fe1125e62921747c6e600b2d803948261R102-R104) ```diff -: `${String(structure.category) - .replace(/([A-Z])/g, " $1") - .trim()} ${structure?.entity_id}`; +: formatCategory(structure.category) + ' ' + structure?.entity_id; ``` Suggestion importance[1-10]: 7Why: Using a utility function for formatting improves code readability and maintainability, but it is not a critical change. | 7 | |
Best practice |
Simplify and correct the application of class names in JSX___ **Ensure consistent usage ofprops.className in the JSX structure to avoid applying styles incorrectly or redundantly.** [client/src/ui/elements/ResourceIcon.tsx [84-85]](https://github.com/BibliothecaDAO/eternum/pull/1069/files#diff-5416bcdc887d471eef7dadc9ac2edb47f9e18e6fb351ed720a1108594efe733bR84-R85) ```diff -
-
+
+
```
Suggestion importance[1-10]: 6Why: Ensuring consistent usage of class names is a good practice for avoiding style issues, but the impact on the overall functionality is minor. | 6 |
PR Type
Enhancement, Bug fix
Description
useStructures.tsx
.InventoryResources.tsx
.StructureListItem.tsx
.ResourceIcon.tsx
.Changes walkthrough π
useStructures.tsx
Enhance structure category name formatting
client/src/hooks/helpers/useStructures.tsx - Improved string formatting for structure category names.
InventoryResources.tsx
Add dynamic resource balance and enhance rendering
client/src/ui/components/resources/InventoryResources.tsx
StructureListItem.tsx
Improve structure list item layout and dynamic resource display
client/src/ui/components/worldmap/structures/StructureListItem.tsx
ResourceIcon.tsx
Refactor imports and fix class name usage in ResourceIcon
client/src/ui/elements/ResourceIcon.tsx