BibliothecaDAO / eternum

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

fix market button #991

Closed ponderingdemocritus closed 1 week ago

ponderingdemocritus commented 1 week ago

PR Type

Bug fix


Description


Changes walkthrough ๐Ÿ“

Relevant files
Bug fix
MarketOrderPanel.tsx
Fix conditional rendering for market order buttons             

client/src/ui/components/trading/MarketOrderPanel.tsx
  • Fixed the conditional rendering logic for the market order buttons.
  • Ensured the correct button displays based on user role and order
    status.
  • +20/-20 

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

    vercel[bot] commented 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 0:57am
    github-actions[bot] commented 1 week ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review [1-5] 2
    ๐Ÿงช Relevant tests No
    ๐Ÿ”’ Security concerns No
    โšก Key issues to review Conditional Logic Reversal:
    The PR reverses the conditional logic for displaying the market order buttons. Ensure that this change correctly handles all scenarios, including edge cases where isSelf might not behave as expected due to asynchronous updates or state changes.
    Async Function Handling:
    The onClick function for the cancel button uses an async function directly. Ensure that there are no unhandled promise rejections or errors that could affect the UI's responsiveness or error handling capabilities.
    github-actions[bot] commented 1 week ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Error handling
    Add error handling for the asynchronous operation in the onClick event ___ **Add error handling for the asynchronous operation within the onClick event of the "cancel"
    button to manage exceptions that might occur during the cancel_order function execution.** [client/src/ui/components/trading/MarketOrderPanel.tsx [274-288]](https://github.com/BibliothecaDAO/eternum/pull/991/files#diff-00bad72a59745d3d904730035fcdeaddda30f53c9212d94f35f277ef202bd0bdR274-R288) ```diff +// Elsewhere in your component +const handleCancel = async () => { + try { + setLoading(true); + await cancel_order({ + signer: account, + trade_id: offer.tradeId, + return_resources: returnResources, + }); + } catch (error) { + console.error('Failed to cancel order:', error); + // Optionally, handle the UI for error state + } finally { + setLoading(false); + } +}; + ```
    Suggestion importance[1-10]: 9 Why: Adding error handling is crucial for robustness, ensuring that any issues during the `cancel_order` function execution are properly managed and logged.
    9
    User experience
    Disable the "cancel" button during loading to prevent multiple submissions ___ **To improve the user experience, consider disabling the "cancel" button while the loading
    state is true, preventing multiple clicks and potential issues with multiple cancellation
    requests.** [client/src/ui/components/trading/MarketOrderPanel.tsx [274-288]](https://github.com/BibliothecaDAO/eternum/pull/991/files#diff-00bad72a59745d3d904730035fcdeaddda30f53c9212d94f35f277ef202bd0bdR274-R288) ```diff ```
    Suggestion importance[1-10]: 8 Why: Disabling the button during loading enhances user experience by preventing multiple clicks and potential issues with multiple cancellation requests.
    8
    Maintainability
    Extract the inline function in the onClick handler to a separate named function ___ **Consider extracting the inline function within the onClick handler of the "cancel" button
    into a separate named function. This will improve readability and maintainability,
    especially if the function logic becomes more complex in the future.** [client/src/ui/components/trading/MarketOrderPanel.tsx [274-288]](https://github.com/BibliothecaDAO/eternum/pull/991/files#diff-00bad72a59745d3d904730035fcdeaddda30f53c9212d94f35f277ef202bd0bdR274-R288) ```diff +// Elsewhere in your component +const handleCancel = async () => { + setLoading(true); + await cancel_order({ + signer: account, + trade_id: offer.tradeId, + return_resources: returnResources, + }); + setLoading(false); +}; + ```
    Suggestion importance[1-10]: 7 Why: Extracting the inline function improves readability and maintainability, making the code easier to manage and test. However, it is not a critical change.
    7
    Simplify the dynamic button text rendering using a function ___ **Consider using a ternary operator or a separate rendering function to manage the button
    text dynamically based on the loading state to simplify the JSX structure.** [client/src/ui/components/trading/MarketOrderPanel.tsx [287]](https://github.com/BibliothecaDAO/eternum/pull/991/files#diff-00bad72a59745d3d904730035fcdeaddda30f53c9212d94f35f277ef202bd0bdR287-R287) ```diff -{loading ? "cancelling" : "cancel"} +{getButtonText()} +// Elsewhere in your component +const getButtonText = () => loading ? "cancelling" : "cancel"; + ```
    Suggestion importance[1-10]: 6 Why: Using a function to manage dynamic button text slightly improves readability but does not significantly impact the overall code quality.
    6