search

BibliothecaDAO / eternum

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

Fix trades y overflow + adjust trades header & rows grid #923

Closed cwastche closed 3 months ago

cwastche commented 3 months ago

PR Type

Enhancement, Bug fix

Description

Changes walkthrough 📝

Relevant files
Enhancement
MarketModal.tsx
Add vertical overflow handling to MarketModal component. 
client/src/ui/components/trading/MarketModal.tsx
  • Added overflow-y-auto class to the main container to handle vertical
    overflow.
    		•  +1/-1     
    MarketOrderPanel.tsx
    Improve MarketOrderPanel layout and add error handling.   
    client/src/ui/components/trading/MarketOrderPanel.tsx
  • Wrapped resource trait in a div with truncate class for text overflow
    handling.
  • Adjusted grid layout for OrderRowHeader and OrderRow.
  • Added error handling and loading state management for order
    acceptance.
  • Improved UI for order rows and buttons.
    		•  +47/-37 

    💡 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 13, 2024 0:37am
    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 `deleteTrade` function is called only after a successful `accept_order` operation. This is good, but there should be a mechanism to handle failures in `accept_order` more gracefully, possibly by showing a user-friendly message or retry option.
    UI Consistency:
    Ensure that the changes in grid layout and class additions like `overflow-y-auto` and `truncate` do not break the existing layout or cause unexpected behavior across different screen sizes or browsers.
    github-actions[bot] commented 3 months ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Improve error handling by ensuring setLoading(false) is called on both success and error ___ **Refactor the onAccept function to handle promise rejection at the top level to improve
    error handling and maintainability.** [client/src/ui/components/trading/MarketOrderPanel.tsx [197-206]](https://github.com/BibliothecaDAO/eternum/pull/923/files#diff-00bad72a59745d3d904730035fcdeaddda30f53c9212d94f35f277ef202bd0bdR197-R206) ```diff .then(() => { deleteTrade(offer.tradeId); }) .catch((error) => { console.error("Failed to accept order", error); + setLoading(false); // Ensure loading is set to false on error }) .finally(() => { setLoading(false); }); ```
    Suggestion importance[1-10]: 10 Why: This suggestion significantly improves error handling by ensuring `setLoading(false)` is called on both success and error, which is crucial for maintaining the correct loading state.
    		10
    Possible bug
    Add conditional rendering to handle potential undefined values ___ **Consider adding a conditional rendering to handle the case when selectedResource is
    undefined or null to prevent runtime errors.** [client/src/ui/components/trading/MarketModal.tsx [87]](https://github.com/BibliothecaDAO/eternum/pull/923/files#diff-60c160393d9cf2b5da3291c081dd0d823b49d1cb1dcdda811544686c0ddd347eR87-R87) ```diff -Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential runtime error by adding a conditional check for `selectedResource`. It is crucial for preventing crashes when `selectedResource` is undefined or null.
    		9
    Ensure type safety for resource.trait in JSX ___ **Ensure that the resource.trait is always a string to prevent type errors in the JSX.
    Consider adding a default value or type assertion.** [client/src/ui/components/trading/MarketOrderPanel.tsx [55]](https://github.com/BibliothecaDAO/eternum/pull/923/files#diff-00bad72a59745d3d904730035fcdeaddda30f53c9212d94f35f277ef202bd0bdR55-R55) ```diff -
    {resource.trait}
    +
    {String(resource.trait)}
    ```
    Suggestion importance[1-10]: 8 Why: Ensuring type safety for `resource.trait` is important to prevent type errors in JSX. This suggestion improves robustness by converting `resource.trait` to a string.
    		8
    Performance
    Use a named function for event handlers to improve performance ___ **Replace inline function in the onClick event with a named function to improve performance
    and readability.** [client/src/ui/components/trading/MarketOrderPanel.tsx [226-233]](https://github.com/BibliothecaDAO/eternum/pull/923/files#diff-00bad72a59745d3d904730035fcdeaddda30f53c9212d94f35f277ef202bd0bdR226-R233) ```diff -onClick={async () => { +onClick={handleCancelOrder} +// Elsewhere in your component +const handleCancelOrder = async () => { setLoading(true); await cancel_order({ signer: account, trade_id: offer.tradeId, return_resources: returnResources, }); setLoading(false); -}} +}; ```
    Suggestion importance[1-10]: 7 Why: Using a named function for event handlers can improve performance and readability. This suggestion is beneficial but not critical.
    		7