search

BibliothecaDAO / eternum

onchain eternal game
https://alpha-eternum.realms.world
MIT License
36 stars 30 forks source link

general fixes and hints #948

Closed ponderingdemocritus closed 3 weeks ago

ponderingdemocritus commented 3 weeks ago

PR Type

enhancement, dependencies, bug fix

Description

Changes walkthrough 📝

Relevant files
Enhancement
10 files
tailwind.config.js
Add custom typography theme and plugin integration.           
client/tailwind.config.js
  • Added custom typography theme with pink color scheme.
  • Included @tailwindcss/typography plugin.
    		•  +39/-0   
    Entity.tsx
    Update CSS classes for spacing and margin.                             
    client/src/ui/components/entities/Entity.tsx - Adjusted CSS classes for spacing and margin.
    		+1/-1     
    Buildings.tsx
    Add Buildings component for displaying building information.
    client/src/ui/components/hints/Buildings.tsx
  • Added new Buildings component to display building information.
  • Included resource costs and building capacities.
    		•  +88/-0   
    GettingStarted.tsx
    Add GettingStarted component with key concepts.                   
    client/src/ui/components/hints/GettingStarted.tsx - Added new `GettingStarted` component with key concepts.
    		+48/-0   
    HintModal.tsx
    Enhance HintModal with new sections and styling.                 
    client/src/ui/components/hints/HintModal.tsx
  • Enhanced HintModal with new sections and components.
  • Updated CSS classes for better styling.
    		•  +102/-54
    TheWorld.tsx
    Add TheWorld component with key concepts.                               
    client/src/ui/components/hints/TheWorld.tsx - Added new `TheWorld` component with key concepts.
    		+25/-0   
    ResourceIcon.tsx
    Update icon for Donkeys resource.                                               
    client/src/ui/elements/ResourceIcon.tsx - Updated icon for `Donkeys` resource.
    		+1/-1     
    BattleProgressBar.tsx
    Optimize calculations and enhance styling in BattleProgressBar.
    client/src/ui/modules/military/battle-view/BattleProgressBar.tsx
  • Optimized calculations using useMemo.
  • Enhanced styling and layout.
    		•  +35/-23 
    NoOngoingBattle.tsx
    Update CSS classes for better styling.                                     
    client/src/ui/modules/military/battle-view/NoOngoingBattle.tsx - Updated CSS classes for better styling.
    		+1/-1     
    OngoingBattle.tsx
    Update CSS classes for better styling.                                     
    client/src/ui/modules/military/battle-view/OngoingBattle.tsx - Updated CSS classes for better styling.
    		+1/-1     
    Bug fix
    1 files
    resources.ts
    Adjust resource costs and fix quest resource amounts.       
    sdk/packages/eternum/src/constants/resources.ts
  • Adjusted resource costs for various building types.
  • Fixed incorrect resource amounts for quests.
    		•  +5/-12   
    Dependencies
    2 files
    package.json
    Add @tailwindcss/typography to devDependencies.                   
    client/package.json - Added `@tailwindcss/typography` to devDependencies.
    		+1/-0     
    pnpm-lock.yaml
    Add @tailwindcss/typography and dependencies.                       
    pnpm-lock.yaml - Added `@tailwindcss/typography` and its dependencies.
    		+31/-0   
    Configuration changes
    2 files
    manifest.json
    Update block number in manifest.                                                 
    contracts/manifests/dev/manifest.json - Updated block number in manifest.
    		+1/-1     
    manifest.toml
    Update block number in manifest.                                                 
    contracts/manifests/dev/manifest.toml - Updated block number in manifest.
    		+1/-1     

    💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    vercel[bot] commented 3 weeks 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 17, 2024 6:07am
    github-actions[bot] commented 3 weeks ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 4
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The integration of the @tailwindcss/typography plugin and the extensive customizations in tailwind.config.js should be carefully reviewed to ensure they do not conflict with existing styles or cause unexpected behavior in different parts of the application.
    Performance Concern:
    The use of useMemo in Buildings.tsx and Resources.tsx for computing derived data could be unnecessary if the computations are not costly or if the dependencies change frequently, leading to potential performance degradation.
    Code Duplication:
    There seems to be a pattern of similar code structures in multiple components like GettingStarted.tsx, TheWorld.tsx, and others. It might be beneficial to abstract common functionalities into reusable components or hooks to reduce redundancy and improve maintainability.
    github-actions[bot] commented 3 weeks ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Accessibility
    Enhance accessibility by adding 'aria-selected' to modal section tabs ___ **For better accessibility and user experience, consider adding 'aria-selected' attribute to
    the section tabs in the modal. This attribute should dynamically reflect the active state
    of the tabs, improving screen reader support.** [client/src/ui/components/hints/HintModal.tsx [68-74]](https://github.com/BibliothecaDAO/eternum/pull/948/files#diff-1dd2daf32164d13d632c2df30c82cc13776cd68a555f9775678593b6abe75783R68-R74) ```diff
    setActiveSection(section)} > ```
    Suggestion importance[1-10]: 10 Why: This suggestion significantly enhances accessibility, making the application more usable for people with disabilities, which is a critical aspect of modern web development.
    		10
    Possible bug
    Add a guard clause to prevent division by zero errors ___ **Consider adding a guard clause to handle the case where totalHealth is zero before
    performing division operations. This will prevent potential division by zero errors, which
    could occur if both attackingHealth.current and defendingHealth.current are zero.** [client/src/ui/modules/military/battle-view/BattleProgressBar.tsx [21-25]](https://github.com/BibliothecaDAO/eternum/pull/948/files#diff-f9a547b6a282b885d288bf085e915af12ff12ce991fc19051cff70106c89c9deR21-R25) ```diff const attackingHealthPercentage = useMemo( - () => ((attackingHealth.current / totalHealth) * 100).toFixed(2), + () => totalHealth > 0 ? ((attackingHealth.current / totalHealth) * 100).toFixed(2) : '0.00', [attackingHealth, totalHealth], ); const defendingHealthPercentage = useMemo( - () => (((defendingHealth?.current || 0) / totalHealth) * 100).toFixed(2), + () => totalHealth > 0 ? (((defendingHealth?.current || 0) / totalHealth) * 100).toFixed(2) : '0.00', [defendingHealth, totalHealth], ); ```
    Suggestion importance[1-10]: 10 Why: This suggestion addresses a potential bug that could cause a division by zero error, which is a critical issue. Adding a guard clause ensures the application handles edge cases gracefully.
    		10
    Performance
    Optimize component rendering by adding necessary dependencies to useMemo ___ **Consider using React.memo or React.useCallback for the 'buildingTable' useMemo dependency
    to avoid unnecessary re-renders when the dependencies have not changed. This enhancement
    can improve performance, especially with larger datasets or more complex UI components.** [client/src/ui/components/hints/Buildings.tsx [15-42]](https://github.com/BibliothecaDAO/eternum/pull/948/files#diff-d23e7afdf462725f12baf43be0191e05f86b7537bab67558541385e7e9081ee9R15-R42) ```diff const buildingTable = useMemo(() => { const buildings = []; ... return buildings; -}, []); +}, [BUILDING_RESOURCE_PRODUCED, BUILDING_COSTS_SCALED, BUILDING_POPULATION, BUILDING_CAPACITY]); ```
    Suggestion importance[1-10]: 9 Why: The suggestion addresses performance optimization, which is crucial for maintaining a responsive user interface, especially with potentially large datasets.
    		9
    Best practice
    Use 'gap-2' instead of 'space-x-2' for consistent spacing in flex containers ___ **Replace the 'space-x-2' class with 'gap-2' in the div element to maintain consistency with
    the new code style. The 'space-x-2' class was removed in the PR, and 'gap-2' was
    introduced, which is a more modern approach with Tailwind CSS for spacing between flex
    items.** [client/src/ui/components/entities/Entity.tsx [100]](https://github.com/BibliothecaDAO/eternum/pull/948/files#diff-e41f7ca60992e606521c1c7a6cda0e416abefc1bc9ec7d0716bc4037a6cb1c43R100-R100) ```diff -
    +
    ```
    Suggestion importance[1-10]: 8 Why: This suggestion ensures consistency in the codebase and adheres to modern best practices, which improves maintainability and readability.
    		8
    Possible issue
    Verify and potentially revert the removal of default resource costs for certain building types ___ **Ensure that the removal of resources for certain building types (e.g., DonkeyFarm,
    TradingPost) is intentional. If these buildings should have a default resource cost,
    consider adding them back or documenting the reason for their removal to avoid confusion.** [sdk/packages/eternum/src/constants/resources.ts [673-674]](https://github.com/BibliothecaDAO/eternum/pull/948/files#diff-03877f9a7de964bc35d454531a120cf998aca1a8b8ebd1d0471d35393c38ac4aR673-R674) ```diff -[BuildingType.DonkeyFarm]: [], -[BuildingType.TradingPost]: [], +[BuildingType.DonkeyFarm]: [{ resource: ResourcesIds.Wheat, amount: 0 }], +[BuildingType.TradingPost]: [{ resource: ResourcesIds.Wheat, amount: 0 }], ```
    Suggestion importance[1-10]: 8 Why: This suggestion highlights a potential issue where resource costs for certain building types were removed. Ensuring these changes are intentional or documenting the reason helps maintain clarity and correctness in the codebase.
    		8
    Enhancement
    Ensure consistent color usage in both normal and inverted modes for better visual cohesion ___ **Consider using a consistent color scheme for both normal and inverted modes to maintain a
    cohesive design language. Currently, the color for '--tw-prose-body' in normal mode is set
    to 'theme("colors.gold")', but in inverted mode, it's set to 'theme("colors.pink[200]")'.
    This inconsistency might cause visual dissonance.** [client/tailwind.config.js [11-27]](https://github.com/BibliothecaDAO/eternum/pull/948/files#diff-c161da30d9a464d0e8adede24e77041c08ea806f0ea5e7547344f60a2e0099fcR11-R27) ```diff "--tw-prose-body": theme("colors.gold"), -"--tw-prose-invert-body": theme("colors.pink[200]"), +"--tw-prose-invert-body": theme("colors.gold"), ```
    Suggestion importance[1-10]: 7 Why: The suggestion improves visual consistency, which is important for user experience, but it is not critical to the functionality of the application.
    		7
    Adjust gradient logic to handle cases where health percentages are equal ___ **The gradient calculation does not account for the scenario where attackingHealthPercentage
    and defendingHealthPercentage are the same, which could visually misrepresent the health
    bars. Consider adjusting the gradient logic to handle this edge case.** [client/src/ui/modules/military/battle-view/BattleProgressBar.tsx [29-31]](https://github.com/BibliothecaDAO/eternum/pull/948/files#diff-f9a547b6a282b885d288bf085e915af12ff12ce991fc19051cff70106c89c9deR29-R31) ```diff const gradient = useMemo( - () => - `linear-gradient(to right, #582C4D ${attackingHealthPercentage}%, rgba(0,0,0,0) ${attackingHealthPercentage}%, rgba(0,0,0,0) ${defendingHealthPercentage}%, #6B7FD7 ${defendingHealthPercentage}%)`, + () => { + const startColor = '#582C4D'; + const endColor = '#6B7FD7'; + return attackingHealthPercentage === defendingHealthPercentage + ? `linear-gradient(to right, ${startColor}, ${endColor})` + : `linear-gradient(to right, ${startColor} ${attackingHealthPercentage}%, rgba(0,0,0,0) ${attackingHealthPercentage}%, rgba(0,0,0,0) ${defendingHealthPercentage}%, ${endColor} ${defendingHealthPercentage}%)`; + }, [attackingHealthPercentage, defendingHealthPercentage], ); ```
    Suggestion importance[1-10]: 7 Why: This suggestion improves the visual representation of the health bars by handling edge cases where health percentages are equal. It enhances the user experience but is not critical to the functionality.
    		7
    Remove or populate the empty className attribute to clean up the code ___ **The className attribute for the motion.div element is set to an empty string, which is
    redundant. Consider removing it or adding appropriate class names if styling is needed.** [client/src/ui/modules/military/battle-view/BattleProgressBar.tsx [39]](https://github.com/BibliothecaDAO/eternum/pull/948/files#diff-f9a547b6a282b885d288bf085e915af12ff12ce991fc19051cff70106c89c9deR39-R39) ```diff -className="" +// className attribute removed ```
    Suggestion importance[1-10]: 5 Why: While this suggestion improves code cleanliness by removing a redundant attribute, it is a minor enhancement and does not impact functionality or performance significantly.
    		5