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/markets #1115

Merged
merged 7 commits into from
Jul 15, 2024
Merged

Fix/markets #1115

merged 7 commits into from
Jul 15, 2024

Conversation

bob0005
Copy link
Collaborator

@bob0005 bob0005 commented Jul 14, 2024

PR Type

Enhancement, Bug fix

Closes #1103
Closes #1109
Closes #1108


Description

  • Added isAmm prop to various components to handle specific logic for AMM.
  • Improved readability of distance calculations by adding parentheses.
  • Enhanced toggle component to handle search terms and adjust toggle logic.
  • Removed unused variables and fixed layout issues in market-related components.
  • Added search functionality and displayed account names in entity selection.
  • Added key prop to TradeHistoryEvent components to fix rendering issues.

Changes walkthrough 📝

Relevant files
Enhancement
8 files
Swap.tsx
Add `isAmm` prop to `ResourceSwap` component                         

client/src/ui/components/bank/Swap.tsx

  • Added isAmm prop to ResourceSwap component.
+1/-0     
RoadRealmSelector.tsx
Improve readability of distance calculation                           

client/src/ui/components/cityview/realm/trade/Roads/RoadRealmSelector.tsx

  • Added parentheses for clarity in distance calculation.
+1/-1     
TradeRealmSelector.tsx
Improve readability of distance calculation                           

client/src/ui/components/cityview/realm/trade/TradeRealmSelector.tsx

  • Added parentheses for clarity in distance calculation.
+1/-1     
SelectLocationPanel.tsx
Improve readability of distance calculation                           

client/src/ui/components/entities/SelectLocationPanel.tsx

  • Added parentheses for clarity in distance calculation.
+1/-1     
ResourceWeight.tsx
Add `isAmm` prop and adjust donkey amount calculation       

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

  • Added isAmm prop to TravelInfo component.
  • Adjusted donkey amount calculation based on isAmm prop.
  • +3/-1     
    ToggleComponent.tsx
    Enhance toggle logic with search term handling                     

    client/src/ui/components/toggle/ToggleComponent.tsx

  • Added useEffect to handle searchTerm prop.
  • Adjusted toggle logic based on searchTerm and open props.
  • +11/-3   
    MarketOrderPanel.tsx
    Improve layout and add account name display                           

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

  • Adjusted layout and styling of MarketResource component.
  • Added account name display in OrderRow component.
  • +21/-15 
    TransferBetweenEntities.tsx
    Add search functionality and display account names             

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

  • Added search functionality for entities.
  • Displayed account names in entity selection.
  • +95/-31 
    Bug fix
    2 files
    MarketModal.tsx
    Remove unused variable and fix layout issue                           

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

  • Removed unused userTrades variable.
  • Fixed layout issue in MarketResourceSidebar.
  • +9/-7     
    MarketTradingHistory.tsx
    Add key prop to TradeHistoryEvent components                         

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

    • Added key prop to TradeHistoryEvent components.
    +2/-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 14, 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 Jul 14, 2024 8:14pm

    Copy link

    PR Reviewer Guide 🔍

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

    Hardcoded Value
    The isAmm prop is hardcoded to true in the ResourceSwap component. Consider making this a dynamic value based on some condition or external input to enhance flexibility.

    State Dependency
    The useEffect hook in ToggleComponent resets isToggled based on props.searchTerm. Ensure that this behavior is intended and does not lead to unexpected toggling in the UI, especially when props.searchTerm changes frequently.

    Search Functionality
    The search functionality in TransferBetweenEntities uses a case-insensitive match for entity names and account names. Ensure that this does not lead to performance issues with a large number of entities, and consider debouncing the input for better performance.

    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, this pull request includes several improvements to the UI components, particularly in the trading and resource management areas. The changes enhance user experience with better layouts, more efficient data handling, and improved search functionality. The code also shows good attention to edge cases and type safety. These updates should make the application more user-friendly and robust.

    Thanks for using MentatBot. Reply with feedback or to ask MentatBot to review other parts of the PR. Please give feedback with emoji reacts.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Correct the onChange handlers to pass only the input value to the state setters

    The onChange handlers for TextInput components in the 'From' and 'To' sections are
    incorrectly passing the entire event to setFromSearchTerm and setToSearchTerm
    instead of just the value.

    client/src/ui/components/trading/TransferBetweenEntities.tsx [164-192]

    -onChange={(fromSearchTerm) => setFromSearchTerm(fromSearchTerm)}
    -onChange={(toSearchTerm) => setToSearchTerm(toSearchTerm)}
    +onChange={(e) => setFromSearchTerm(e.target.value)}
    +onChange={(e) => setToSearchTerm(e.target.value)}
     
    Suggestion importance[1-10]: 10

    Why: This suggestion fixes a clear bug in the onChange handlers, ensuring that only the input value is passed to the state setters. This is a critical fix for the functionality of the component.

    10
    Correct the dependency array in useMemo for accountName to depend on offer.makerId

    The dependency array of useMemo for accountName incorrectly includes
    offer.originName instead of offer.makerId. This could lead to unexpected behavior if
    offer.makerId changes but offer.originName does not.

    client/src/ui/components/trading/MarketOrderPanel.tsx [280-282]

     const accountName = useMemo(() => {
         return getRealmAddressName(offer.makerId);
    -  }, [offer.originName]);
    +  }, [offer.makerId]);
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential bug that could lead to unexpected behavior. Correcting the dependency array is crucial for ensuring the correct memoization behavior.

    9
    Maintainability
    Refactor the calculation of currentDonkeyAmount to a function for better clarity and maintainability

    Instead of setting currentDonkeyAmount to 0 when isAmm is true, consider using a
    more descriptive variable or method that explains why currentDonkeyAmount should be
    0 in this context.

    client/src/ui/components/resources/ResourceWeight.tsx [32]

    -const currentDonkeyAmount = isAmm ? 0 : resources.find((r) => r.resourceId === ResourcesIds.Donkey)?.amount || 0;
    +const currentDonkeyAmount = shouldResetDonkeyAmount(isAmm, resources);
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves code readability and maintainability by encapsulating logic in a function. However, it is not addressing a critical issue, so it receives a moderate score.

    7
    Enhancement
    Make the isAmm prop dynamic to increase component flexibility

    Consider using a prop to determine the value of isAmm dynamically based on
    conditions or props rather than hardcoding it to true. This will make the component
    more flexible and reusable.

    client/src/ui/components/bank/Swap.tsx [148]

    -isAmm={true}
    +isAmm={someCondition ? true : false}
     
    Suggestion importance[1-10]: 5

    Why: While making the isAmm prop dynamic could increase flexibility, the suggestion is somewhat vague and does not provide a specific condition or context for the change. It is a minor enhancement rather than a crucial improvement.

    5

    Copy link
    Collaborator

    @edisontim edisontim left a comment

    Choose a reason for hiding this comment

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

    lgtm

    @edisontim edisontim merged commit 1e40ae5 into main Jul 15, 2024
    19 checks passed
    @edisontim edisontim deleted the fix/markets branch July 15, 2024 09:46
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    2 participants