Closed edisontim closed 1 week 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 24, 2024 3:39pm |
⏱️ Estimated effort to review [1-5] | 3 |
🧪 Relevant tests | No |
🔒 Security concerns | No |
⚡ Key issues to review |
Possible Bug: The ResourceInfo component now accepts extraButtons as a prop, but the implementation in HoverBuilding does not pass any buttons to it when the building type is Resource . This might be an oversight if the intention was to allow additional actions on resource buildings. |
Code Clarity: The conditional rendering logic in HoverBuilding can be simplified to improve readability and maintainability. Consider consolidating the conditions or restructuring the component logic to make it clearer which components are rendered based on the building type. |
Category | Suggestion | Score |
Maintainability |
Replace nested ternary operators with a mapping object for clarity___ **Refactor the nested ternary operators in theMiddleBuilding component to use a switch statement or a mapping object for better readability and maintainability.** [client/src/ui/components/construction/ExistingBuildings.tsx [134-142]](https://github.com/BibliothecaDAO/eternum/pull/1002/files#diff-b8b13d61f7fc906dae1ff3473650c5893b1d3971d1b65e062a1fb4c6ed5e749bR134-R142) ```diff -hexType === HexType.BANK - ? ModelsIndexes.Bank - : hexType === HexType.SHARDSMINE - ? ModelsIndexes.FragmentMine - : hexType === HexType.HYPERSTRUCTURE - ? ModelsIndexes.Hyperstructure - : hexType === HexType.UNFINISHEDHYPERSTRUCTURE - ? ModelsIndexes.UnfinishedHyperstructure - : ModelsIndexes.Castle; +const buildingIndexMap = { + [HexType.BANK]: ModelsIndexes.Bank, + [HexType.SHARDSMINE]: ModelsIndexes.FragmentMine, + [HexType.HYPERSTRUCTURE]: ModelsIndexes.Hyperstructure, + [HexType.UNFINISHEDHYPERSTRUCTURE]: ModelsIndexes.UnfinishedHyperstructure, + default: ModelsIndexes.Castle, +}; +return buildingIndexMap[hexType] || buildingIndexMap.default; ``` Suggestion importance[1-10]: 8Why: Refactoring the nested ternary operators into a mapping object significantly improves readability and maintainability, making the code easier to understand and modify. | 8 |
Extract inline style calculation to a function___ **Extract the inline style calculation forclassName in BaseThreeTooltip into a separate function to improve code readability and maintainability.** [client/src/ui/components/construction/ExistingBuildings.tsx [307]](https://github.com/BibliothecaDAO/eternum/pull/1002/files#diff-b8b13d61f7fc906dae1ff3473650c5893b1d3971d1b65e062a1fb4c6ed5e749bR307-R307) ```diff -className={`min-w-[${buildingType === BuildingType.Resource ? 400 : 215}px]`} +className={getTooltipWidth(buildingType)} +function getTooltipWidth(buildingType) { + return `min-w-[${buildingType === BuildingType.Resource ? 400 : 215}px]`; +} + ``` Suggestion importance[1-10]: 7Why: Extracting the inline style calculation into a separate function improves readability and maintainability, but it is a minor enhancement. | 7 | |
Improve variable naming for clarity___ **Consider using a more descriptive variable name instead ofhexType in the MiddleBuilding component to improve code readability. The name buildingType might be more appropriate since it directly relates to the type of building being processed.** [client/src/ui/components/construction/ExistingBuildings.tsx [134]](https://github.com/BibliothecaDAO/eternum/pull/1002/files#diff-b8b13d61f7fc906dae1ff3473650c5893b1d3971d1b65e062a1fb4c6ed5e749bR134-R134) ```diff -export const MiddleBuilding = ({ hexType }: { hexType: HexType }) => { +export const MiddleBuilding = ({ buildingType }: { buildingType: HexType }) => { ``` Suggestion importance[1-10]: 6Why: The suggestion to rename `hexType` to `buildingType` improves code readability and clarity, but it is a minor change and does not address any critical issues. | 6 | |
Best practice |
Use named constants instead of magic numbers for CSS values___ **Replace the magic numbers in theclassName for min-w-[...] with named constants to improve code readability and maintainability.** [client/src/ui/components/construction/SelectPreviewBuilding.tsx [307]](https://github.com/BibliothecaDAO/eternum/pull/1002/files#diff-c6a222463602fe87932fbbc060d2d99cdd1b151dcb9fcd9be42b602a77f18d97R307-R307) ```diff -className={`min-w-[${buildingType === BuildingType.Resource ? 400 : 215}px]`} +const MIN_WIDTH_RESOURCE = 400; +const MIN_WIDTH_OTHER = 215; +className={`min-w-[${buildingType === BuildingType.Resource ? MIN_WIDTH_RESOURCE : MIN_WIDTH_OTHER}px]`} ``` Suggestion importance[1-10]: 7Why: Replacing magic numbers with named constants improves code readability and maintainability, but it is a minor improvement and does not address any critical issues. | 7 |
User description
Closes #996
PR Type
enhancement
Description
HoverBuilding
component to display ongoing costs for resource buildings using theResourceInfo
component.ResourceInfo
component to support additional buttons.BottomMiddleContainer
to avoid interaction issues.BottomNavigation
for better user interaction.Changes walkthrough 📝
ExistingBuildings.tsx
Enhance building tooltip to show ongoing costs for resource buildings.
client/src/ui/components/construction/ExistingBuildings.tsx
ResourceInfo
component.HoverBuilding
component to displayResourceInfo
for resourcebuildings.
information.
SelectPreviewBuilding.tsx
Extend `ResourceInfo` component to support extra buttons.
client/src/ui/components/construction/SelectPreviewBuilding.tsx
extraButtons
prop toResourceInfo
component.ResourceInfo
to include extra buttons in the UI.BottomMiddleContainer.tsx
Prevent pointer events on BottomMiddleContainer.
client/src/ui/containers/BottomMiddleContainer.tsx - Added `pointer-events-none` class to container div.
BottomNavigation.tsx
Enable pointer events on BottomNavigation.
client/src/ui/modules/navigation/BottomNavigation.tsx - Added `pointer-events-auto` class to navigation div.