Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: balance resource view #1145

Merged
merged 4 commits into from
Aug 1, 2024
Merged

fix: balance resource view #1145

merged 4 commits into from
Aug 1, 2024

Conversation

edisontim
Copy link
Collaborator

@edisontim edisontim commented Jul 29, 2024

Fixes a bug in the setInterval function that updates the resource balance in the RightNavigationModule, now updates dynamically the balance every second.

Continues the style transfer --> Closes #1112

PR Type

Bug fix, Enhancement, Formatting


Description

  • Simplified balance calculation logic in ProductionManager.
  • Updated resource inputs and outputs in building preview.
  • Fixed and optimized resource balance update interval in ResourceChip.
  • Updated styling and import order in MarketModal.
  • Updated background color and button styling in battle view.

Changes walkthrough 📝

Relevant files
Enhancement
ProductionManager.ts
Simplify balance calculation logic                                             

client/src/dojo/modelManager/ProductionManager.ts

  • Simplified balance calculation using Math.min.
  • Removed redundant empty lines.
  • +1/-3     
    SelectPreviewBuilding.tsx
    Update resource inputs and outputs in building preview     

    client/src/ui/components/construction/SelectPreviewBuilding.tsx

  • Replaced scaled resource inputs and outputs with regular ones.
  • Added RESOURCE_INPUTS import.
  • +4/-3     
    Bug fix
    ResourceChip.tsx
    Fix and optimize resource balance update interval               

    client/src/ui/components/resources/ResourceChip.tsx

  • Fixed interval timing for balance updates.
  • Simplified balance update logic.
  • Reordered imports for consistency.
  • +7/-9     
    Formatting
    MarketModal.tsx
    Update MarketModal styling and import order                           

    client/src/ui/components/trading/MarketModal.tsx

  • Changed background color and style of modal container.
  • Reordered useGetBanks import.
  • +3/-3     
    TopScreenView.tsx
    Update battle view styling                                                             

    client/src/ui/modules/military/battle-view/TopScreenView.tsx

  • Updated background color and style of battle view.
  • Added button styling.
  • +4/-2     

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

    Copy link

    vercel bot commented Jul 29, 2024

    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 Aug 1, 2024 0:18am

    Copy link
    Contributor

    @mentatbot mentatbot bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Overall, the changes look good and seem to improve both functionality and UI. The main points to be cautious about are the changes in resource output calculation and the update interval for the ResourceChip component. These might need thorough testing to ensure they don't introduce any unexpected behavior. The UI changes, particularly in the MarketModal, should enhance the visual appeal and consistency of the application.

    Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Refactoring Logic
    The refactoring of balance calculation using Math.min simplifies the logic but needs verification to ensure it handles all edge cases correctly, especially under different production and depletion scenarios.

    Data Source Change
    The change from RESOURCE_OUTPUTS_SCALED to RESOURCE_OUTPUTS and from RESOURCE_INPUTS_SCALED to RESOURCE_INPUTS might affect the calculations if the scaling factors are different. This needs a thorough review to ensure that the building's resource management behaves as expected.

    Interval Adjustment
    The update interval for the resource balance display has been changed from 2 milliseconds to 1000 milliseconds. This change in frequency can impact the user experience by making the balance updates less smooth. It's crucial to validate this change with user experience considerations.

    Copy link

    github-actions bot commented Jul 29, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Accessibility
    Enhance accessibility by adding an aria-label to the button

    Add an aria-label to the button for better accessibility, ensuring it is usable by
    all users including those using screen readers.

    client/src/ui/modules/military/battle-view/TopScreenView.tsx [21]

    -<Button className="bg-gold/20 text-white" onClick={() => setBattleView(null)}>
    +<Button className="bg-gold/20 text-white" onClick={() => setBattleView(null)} aria-label="Exit Battle View">
     
    Suggestion importance[1-10]: 10

    Why: Adding an aria-label significantly improves accessibility, making the button more usable for screen reader users. This is a crucial improvement for inclusivity.

    10
    Maintainability
    Improve code clarity by using Math.abs(rate) instead of double negation

    Instead of using a unary minus to negate the negative rate, directly use the
    positive value of rate for clarity and to avoid potential confusion or errors.

    client/src/dojo/modelManager/ProductionManager.ts [132]

    -let balance = Number(resource?.balance || 0n) - -this._depletionDuration(currentTick, resourceId) * rate;
    +let balance = Number(resource?.balance || 0n) - this._depletionDuration(currentTick, resourceId) * Math.abs(rate);
     
    Suggestion importance[1-10]: 9

    Why: The suggestion improves code readability and avoids potential confusion by using Math.abs(rate) instead of a double negation. This change is logical and enhances maintainability.

    9
    Use a theme management approach for styling to enhance maintainability

    Replace the hardcoded style classes with a theme-based approach or use a constant to
    manage UI themes, improving maintainability and theme consistency.

    client/src/ui/components/trading/MarketModal.tsx [89]

    -<div className="container border mx-auto  grid grid-cols-12 bg-black/90 bg-hex-bg border-gold/30 clip-angled h-full row-span-12 ">
    +<div className={themeStyles.container}>
     
    Suggestion importance[1-10]: 6

    Why: Using a theme-based approach for styling can improve maintainability and consistency. However, the suggestion lacks details on how the themeStyles.container is defined and managed.

    6
    Performance
    Increase the interval duration to improve performance

    Update the interval duration to a more appropriate value to avoid rapid state
    updates which can lead to performance issues.

    client/src/ui/components/resources/ResourceChip.tsx [61]

    -}, 1000);
    +}, 2000);
     
    Suggestion importance[1-10]: 7

    Why: Increasing the interval duration can help improve performance by reducing the frequency of state updates. However, the new interval value should be carefully chosen to balance performance and responsiveness.

    7

    @edisontim edisontim requested a review from bob0005 July 29, 2024 12:06
    @bob0005 bob0005 merged commit 8ed26fb into main Aug 1, 2024
    20 of 21 checks passed
    @bob0005 bob0005 deleted the fix/balance-resource-view branch August 1, 2024 16:00
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Complete style transfer
    2 participants