Closed ponderingdemocritus closed 1 month ago
vercel[bot]
commented
1 month 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 13, 2024 10:10am |
github-actions[bot]
commented
1 month ago | ⏱️ Estimated effort to review [1-5] | 4 |
| 🧪 Relevant tests | No |
| 🔒 Security concerns | No |
| ⚡ Key issues to review |
Possible Inconsistency: The PR introduces changes to various components, including styling and functionality. However, it's not clear if all changes are consistent across different components and if they adhere to the design system of the application. |
|
Code Clarity: There are several commented-out lines of code, such as the removal of the StaminaResource component in some places while it's being added or modified in others. This could lead to confusion and potential bugs if not properly managed. |
github-actions[bot]
commented
1 month ago | Category | Suggestion | Score |
| Possible bug |
Add error handling for non-numeric army value conversion to BigInt___ **The division operation for displaying the army's health points (hp) might throw an errorif army.value.toString() results in a non-numeric string. Ensure that the value is always numeric or handle potential exceptions.** [client/src/ui/components/military/ArmyChip.tsx [25]](https://github.com/BibliothecaDAO/eternum/pull/921/files#diff-9392efc502d8dd91125b44c0f8a38789aa297e674db0538a8893fb6ab9bd7487R25-R25) ```diff - hp: {(BigInt(army.value.toString()) / BigInt(1000n)).toString()}
+hp: {isNaN(army.value) ? 'Invalid' : (BigInt(army.value.toString()) / BigInt(1000n)).toString()}
```
Suggestion importance[1-10]: 9Why: This suggestion addresses a potential bug that could cause runtime errors. Adding error handling for non-numeric values is crucial for robustness, making this a high-impact improvement. | 9 |
| Best practice |
Replace the use of
___
**It's recommended to avoid using the | 8 |
| Layout adjustment |
Adjust the grid gap to accommodate the new border styling in troop display components___ **The addition of borders to the troop display components increases the visual separation,which might affect the overall layout. Consider adjusting the grid's gap to maintain visual balance.** [client/src/ui/components/military/TroopChip.tsx [8-16]](https://github.com/BibliothecaDAO/eternum/pull/921/files#diff-cbece6b978b65197edd5a82b070123d6ae7ad014017f363d3291a958e99803f5R8-R16) ```diff -className="px-2 py-1 bg-white/10 clip-angled-sm flex flex-col justify-between gap-2 border border-gold/10" +className="px-2 py-1 bg-white/10 clip-angled-sm flex flex-col justify-between gap-3 border border-gold/10" ``` Suggestion importance[1-10]: 7Why: Adjusting the grid gap to maintain visual balance is a good suggestion for layout consistency. It addresses a minor but noticeable aspect of the UI, making it a useful enhancement. | 7 |
| Enhancement |
Verify the new border and padding styles with the design specifications___ **TheclassName property for the troop display div has been changed to include borders and padding. Consider verifying with the design team if the visual changes align with the intended design, especially the border color and padding adjustments.** [client/src/ui/components/military/ArmyManagementCard.tsx [235]](https://github.com/BibliothecaDAO/eternum/pull/921/files#diff-58b0b7210df6fa085fddcc914d734b935da99558172e2c10c60a2d4398bd13b9R235-R235) ```diff -className="p-3 bg-gold/10 clip-angled-sm hover:bg-gold/30 flex flex-col border-gold/10 border" +className="p-2 bg-gold/10 clip-angled-sm hover:bg-gold/30 flex flex-col border-gray-300 border" ``` Suggestion importance[1-10]: 5Why: While the suggestion to verify design changes is valid, it is more of a procedural recommendation rather than a direct code improvement. It is important but not critical. | 5 |
PR Type
enhancement, other
Description
ResourceCostcomponent inEntity.tsx.EntityList.tsx.ArmyChipcomponent with new styles andStaminaResource.StaminaResourceinArmyListcomponent.ArmyManagementCardcomponent.TroopChipcomponent with border styling.InventoryResourcescomponent with gap styling.ResourceCostcomponent with new styles.StaminaResourcecomponent.Onboardinglayout styling.TopLeftNavigationtoWorldlayout.BottomNavigation.TopLeftNavigationcomponent with hint and settings buttons.TopMiddleNavigationcomponent with layout adjustments.Changes walkthrough 📝
13 files
Entity.tsx
Simplified className for ResourceCost component.client/src/ui/components/entities/Entity.tsx
ResourceCostcomponent.EntityList.tsx
Added border styling to EntityList items.client/src/ui/components/list/EntityList.tsx - Added border styling to list items.
ArmyChip.tsx
Enhanced ArmyChip component with new styles and StaminaResource.client/src/ui/components/military/ArmyChip.tsx
StaminaResourcecomponent.ArmyManagementCard.tsx
Updated styling for ArmyManagementCard component.client/src/ui/components/military/ArmyManagementCard.tsx - Adjusted grid gap and added border styling for troops.
TroopChip.tsx
Enhanced TroopChip component with border styling.client/src/ui/components/military/TroopChip.tsx - Added border styling to troop resource items.
InventoryResources.tsx
Enhanced InventoryResources component with gap styling.client/src/ui/components/resources/InventoryResources.tsx - Added gap styling to inventory resources.
ResourceCost.tsx
Enhanced ResourceCost component with new styles.client/src/ui/elements/ResourceCost.tsx
StaminaResource.tsx
Enhanced StaminaResource component with updated styles.client/src/ui/elements/StaminaResource.tsx - Updated text color and layout for StaminaResource component.
Onboarding.tsx
Simplified Onboarding layout styling.client/src/ui/layouts/Onboarding.tsx - Removed ornate borders from Onboarding layout.
World.tsx
Added TopLeftNavigation to World layout.client/src/ui/layouts/World.tsx - Added TopLeftContainer and TopLeftNavigation components.
BottomNavigation.tsx
Removed settings button from BottomNavigation.client/src/ui/modules/navigation/BottomNavigation.tsx - Removed settings button from BottomNavigation.
TopLeftNavigation.tsx
Added TopLeftNavigation component.client/src/ui/modules/navigation/TopLeftNavigation.tsx
TopMiddleNavigation.tsx
Enhanced TopMiddleNavigation component with layout adjustments.client/src/ui/modules/navigation/TopMiddleNavigation.tsx - Adjusted layout and added primary variant to button.
1 files
ArmyList.tsx
Commented out StaminaResource in ArmyList component.client/src/ui/components/military/ArmyList.tsx - Commented out `StaminaResource` component.