-
Notifications
You must be signed in to change notification settings - Fork 3
fix: hidden buttons wallet edit form #1096
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
Conversation
📝 WalkthroughWalkthroughForm container styling changed from a fixed-height, scrollable block to a flexible column layout. The wallet edit form replaces inline address rendering with a checkbox list for selectable addresses and integrates react-hook-form registrations for Default XEC/BCH toggles inside a scrollable form area. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (1)
components/Wallet/EditWalletForm.tsx (1)
70-71: Remove unused emptyuseEffect.This
useEffecthas no body and serves no purpose. Removing it will improve code clarity.🔎 Proposed fix
- useEffect(() => { - }, [selectedAddressIdList]) -
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/Paybutton/paybutton.module.csscomponents/Wallet/EditWalletForm.tsx
🧰 Additional context used
🪛 Biome (2.1.2)
components/Wallet/EditWalletForm.tsx
[error] 113-113: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (2)
components/Paybutton/paybutton.module.css (1)
148-149: LGTM! CSS change supports the scrollable inner container.The transition from fixed-height scrolling to a flexible column layout aligns with the new scrollable wrapper introduced in
EditWalletForm.tsx. This ensures action buttons remain visible while the form content scrolls.components/Wallet/EditWalletForm.tsx (1)
90-90: LGTM! Scrollable wrapper fixes the hidden buttons issue.The scrollable container with
maxHeight: 'calc(90vh - 150px)'ensures form content scrolls independently while action buttons remain visible at the bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
components/Wallet/EditWalletForm.tsx (1)
112-114: Use a unique key for button pill elements.
conn.addressIdis the same for all paybuttons belonging to the same address, so it won't be unique within this inner map. Useconn.idor a composite key instead to avoid React reconciliation issues.🔎 Proposed fix
{addr.paybuttons.map((conn) => ( - <div key={conn.addressId} className={style.buttonpill}>{conn.paybutton.name}</div> + <div key={conn.id} className={style.buttonpill}>{conn.paybutton.name}</div> ))}
🧹 Nitpick comments (1)
components/Wallet/EditWalletForm.tsx (1)
70-71: Remove emptyuseEffect.This effect has no body and serves no purpose—it's dead code that should be removed.
🔎 Proposed fix
- useEffect(() => { - }, [selectedAddressIdList]) -
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/Wallet/EditWalletForm.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (3)
components/Wallet/EditWalletForm.tsx (3)
90-99: Scrollable container and name input look good.The inline style with
maxHeight: 'calc(90vh - 150px)'appropriately ensures the form content scrolls while keeping action buttons visible, addressing the original issue.
100-119: Address checkbox list implementation is correct.The checkbox list properly initializes from
thisWalletAddressIdListand wires tohandleSelectedAddressesChange. Keys on the outerdivelements are unique viaaddr.id.
121-145: Default wallet toggles are correctly implemented.The
htmlForattributes now match the corresponding inputidvalues (isXECDefaultandisBCHDefault), fixing the previous accessibility issues. The conditional rendering based onusedNetworksand the disabled state logic are appropriate.
Related to #1094
Description
Fix wallet edit form to stop hiding the action buttons.
Test plan
Go to the Wallets page, try editing a wallet with at least six addresses, and make sure the buttons are not hidden when scrolling to the bottom.
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.