search

BibliothecaDAO / eternum

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

Fixes notification position + adds armies with moves left notification #933

Closed cwastche closed 3 months ago

cwastche commented 3 months ago

PR Type

Enhancement, Bug fix Closes #914

Description

Changes walkthrough 📝

Relevant files
Bug fix
EntitiesArmyTable.tsx
Fix key props in EntitiesArmyTable component.                       
client/src/ui/components/military/EntitiesArmyTable.tsx
  • Fixed key prop for ArmyChip component.
  • Added key prop to the entity container div.
    		•  +2/-2     
    Enhancement
    CircleButton.tsx
    Add notification location prop to CircleButton component.
    client/src/ui/elements/CircleButton.tsx
  • Added notificationLocation prop to CircleButton.
  • Implemented dynamic notification positioning.
  • Wrapped button in a relative div for positioning.
    		•  +17/-3   
    BottomNavigation.tsx
    Set notification location for CircleButton in BottomNavigation.
    client/src/ui/modules/navigation/BottomNavigation.tsx
  • Set notificationLocation prop for CircleButton in BottomNavigation.
    		•  +1/-0     
    LeftNavigationModule.tsx
    Add army stamina check and notification in LeftNavigationModule.
    client/src/ui/modules/navigation/LeftNavigationModule.tsx
  • Removed unused useEffect import.
  • Added imports for useEntityArmies and useStamina.
  • Filtered armies with stamina left.
  • Set notification and notificationLocation props for CircleButton.
    		•  +16/-2   
    RightNavigationModule.tsx
    Set notification location for CircleButton in RightNavigationModule.
    client/src/ui/modules/navigation/RightNavigationModule.tsx
  • Set notificationLocation prop for CircleButton in
    RightNavigationModule.
    		•  +1/-0     

    💡 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 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 14, 2024 1:58pm
    github-actions[bot] commented 3 months ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 3
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The `notificationLocation` prop in `CircleButton` component is set to default "topleft" which might not be suitable for all use cases. It's important to ensure that this default setting is appropriate across all instances where `CircleButton` is used or consider making it a required prop without a default to enforce explicit configuration.
    Code Duplication:
    The logic for calculating notification positions in `CircleButton` is directly embedded within the component. This could be refactored into a utility function or custom hook to improve code reuse and maintainability.
    github-actions[bot] commented 3 months ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Enhance the uniqueness of the key attribute in the list component ___ **Replace the hardcoded key attribute with a more unique identifier by combining
    army.entity_id with another unique property of the army, such as army.army_id, to avoid
    potential key collisions in lists where armies might have the same entity_id but different
    army_id.** [client/src/ui/components/military/EntitiesArmyTable.tsx [20]](https://github.com/BibliothecaDAO/eternum/pull/933/files#diff-c54efdb5349a28b5997ea9a8de6e9bcca6c2f3acea25281f0ea2c8ede9e0c08bR20-R20) ```diff -return ; +return ; ```
    Suggestion importance[1-10]: 8 Why: This suggestion improves the uniqueness of the key attribute, which is crucial for React's reconciliation process and helps prevent potential key collisions in lists.
    		8
    Possible bug
    Implement error handling for resource arrival data retrieval ___ **Add error handling for getAllArrivalsWithResources().length to manage cases where
    getAllArrivalsWithResources() might return null or undefined, preventing runtime errors.** [client/src/ui/modules/navigation/RightNavigationModule.tsx [122]](https://github.com/BibliothecaDAO/eternum/pull/933/files#diff-8b98807e05dc7525757f9d43f5a3b1a081305f54aa3be0d5be4b2a130160bd0cR122-R122) ```diff -notification={getAllArrivalsWithResources().length} +notification={getAllArrivalsWithResources() ? getAllArrivalsWithResources().length : 0} ```
    Suggestion importance[1-10]: 8 Why: Implementing error handling for `getAllArrivalsWithResources().length` is important to prevent runtime errors when the function returns `null` or `undefined`, ensuring the application remains robust.
    		8
    Add validation for armiesWithStaminaLeft.length to ensure it is a valid number ___ **Consider using a more robust type check or validation for armiesWithStaminaLeft.length
    before using it as a notification number to ensure it does not unintentionally pass
    incorrect values like undefined or null.** [client/src/ui/modules/navigation/LeftNavigationModule.tsx [111]](https://github.com/BibliothecaDAO/eternum/pull/933/files#diff-be9e8648ba688d13b3b68afca51d87c31cca8d9412f8c7d77edee0d71737a287R111-R111) ```diff -notification={armiesWithStaminaLeft.length} +notification={Number.isInteger(armiesWithStaminaLeft.length) ? armiesWithStaminaLeft.length : 0} ```
    Suggestion importance[1-10]: 7 Why: Adding validation for `armiesWithStaminaLeft.length` ensures that the notification number is always a valid integer, preventing potential runtime errors.
    		7