Mattupham/fe 726 porfolio v3 alloyed convert button deposit withdraw dropdown #3841
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/web/localizations/en.jsonis excluded by!**/*.json
Files selected for processing (1)
- packages/web/components/table/asset-balances.tsx (4 hunks)
Additional comments not posted (2)
packages/web/components/table/asset-balances.tsx (2)
509-510: Enhancements toAssetActionsCellcomponent look good!The changes in the
AssetActionsCellcomponent enhance its functionality to handle asset conversion scenarios. The new propscoinMinimalDenomandvariantGroupKeyare used to determine if conversion is needed. The rendering logic is updated to conditionally display the "Convert" button when conversion is necessary, and the deposit and withdraw buttons are shown only when conversion is not needed.These changes improve the component's behavior and provide a better user experience by showing relevant actions based on the asset's conversion state.
Also applies to: 518-519, 525-530, 547-595
627-663: NewAssetActionsDropdowncomponent looks great!The introduction of the
AssetActionsDropdowncomponent is a positive addition to the codebase. It encapsulates the dropdown functionality for asset actions, making it a reusable and modular component.The component receives
actionOptionsandonSelectActionas props, allowing flexibility in defining the available actions and handling the selection of an action. It utilizes thePopovercomponent to implement the dropdown behavior and renders the action options as buttons within the dropdown.The component is well-structured, properly implemented, and integrates seamlessly with the existing code. It improves the overall organization and maintainability of the asset actions functionality.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/web/localizations/en.jsonis excluded by!**/*.jsonpackages/web/public/icons/sprite.svgis excluded by!**/*.svg,!**/*.svg
Files selected for processing (1)
- packages/web/components/table/asset-balances.tsx (6 hunks)
Additional comments not posted (5)
packages/web/components/table/asset-balances.tsx (5)
159-160: See the previous comment about removing console log statements.
511-512: LGTM!The code changes are approved. The new props
coinMinimalDenomandvariantGroupKeyare added to theAssetActionsCellcomponent to determine if asset conversion is necessary.
515-523: LGTM!The code changes are approved. The
needsConversionvariable is correctly introduced to check ifcoinMinimalDenomdiffers fromvariantGroupKey. TheshowConvertButtonvariable is correctly introduced to conditionally show the convert button based on thealloyedAssetsfeature flag andneedsConversionstate.
524-563: LGTM!The code changes are approved. The
getActionOptionsfunction is correctly introduced to return an array of action options based on theshowConvertButtonstate. ThehandleSelectActionfunction is correctly introduced to handle the routing logic when an action is selected from the dropdown.
640-701: LGTM!The code changes are approved. The new
AssetActionsDropdowncomponent is correctly introduced to encapsulate the dropdown functionality for action options. The dropdown is correctly populated with action options that are conditionally displayed based on the user's context. The dropdown correctly integrates with the routing logic to navigate to the appropriate pages when actions are selected. The component enhances the modularity of the code and improves the organization of action-related UI elements.
mattupham
changed the title
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| e.preventDefault(); | ||
| // TODO - open conversion modal once clicked |
There was a problem hiding this comment.
will plug in the modal in the next PR
There was a problem hiding this comment.
This is behind a flag right?
There was a problem hiding this comment.
yeah! calculated here - const showConvertButton = featureFlags.alloyedAssets && needsConversion;
jonator
force-pushed
the
stage
branch
2 times, most recently
from
dc35fb0
to
c0894d0
Compare
…t-button-deposit-withdraw-dropdown
| const handleSelectAction = ( | ||
| action: string, | ||
| coinDenom: string, | ||
| router: NextRouter, | ||
| bridgeAsset: ({ | ||
| anyDenom, | ||
| direction, | ||
| }: { | ||
| anyDenom: string | undefined; | ||
| direction: "deposit" | "withdraw" | undefined; | ||
| }) => void | ||
| ) => { |
There was a problem hiding this comment.
Specify the action parameter type in handleSelectAction.
The action parameter is currently typed as string. To enhance type safety and prevent potential bugs from invalid action keys, consider typing it as the Action type defined earlier.
Apply this diff to update the parameter type:
const handleSelectAction = (
- action: string,
+ action: Action,
coinDenom: string,
router: NextRouter,
bridgeAsset: ({
anyDenom,
direction,
}: {
anyDenom: string | undefined;
direction: "deposit" | "withdraw" | undefined;
}) => void
) => {This ensures that only valid actions of type Action are passed to the function, improving code robustness.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleSelectAction = ( | |
| action: string, | |
| coinDenom: string, | |
| router: NextRouter, | |
| bridgeAsset: ({ | |
| anyDenom, | |
| direction, | |
| }: { | |
| anyDenom: string | undefined; | |
| direction: "deposit" | "withdraw" | undefined; | |
| }) => void | |
| ) => { | |
| const handleSelectAction = ( | |
| action: Action, | |
| coinDenom: string, | |
| router: NextRouter, | |
| bridgeAsset: ({ | |
| anyDenom, | |
| direction, | |
| }: { | |
| anyDenom: string | undefined; | |
| direction: "deposit" | "withdraw" | undefined; | |
| }) => void | |
| ) => { |
mattupham
deleted the
mattupham/fe-726-porfolio-v3-alloyed-convert-button-deposit-withdraw-dropdown
branch
* Amplitude: prevent outlier value usd logs (#3825) * prevent outlier value usd logs * usd const * forgot one * format * Mattupham/fe 726 porfolio v3 alloyed convert button deposit withdraw dropdown (#3841) * Add convert button * Add convert button * Clean up * Handle feature via flag * Update icons * Clean up * Clean up * i18n * Extract functionality * Extract type * Clean up comments * Refactor icon * Clean up styles * Update types * fix: amino signing for withdraw position message (#3852) * add favorites to portfolio (#3853) * Update sprite * Clean up * Update to useUserWatchlist --------- Co-authored-by: Jon Ator <[email protected]> Co-authored-by: Matt Upham <[email protected]> Co-authored-by: PaddyMc <[email protected]>
What is the purpose of the change:
Linear Task
https://linear.app/osmosis/issue/FE-726/porfolio-v3-alloyed-convert-button-deposit-withdraw-dropdown
https://linear.app/osmosis/issue/FE-1061/portfolio-v3-portfolioalloyed-feature-flag
Brief Changelog
Testing and Verifying