-
Notifications
You must be signed in to change notification settings - Fork 3
fix: Shortened addresses on Transaction html table at lower res #1085
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
WalkthroughThis PR adds centralized address formatting and clipboard copy functionality to PayButton components. It introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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: 0
🧹 Nitpick comments (3)
components/Transaction/PaybuttonTransactions.tsx (2)
1-2: Clipboard copy flow is well-structured and cleaned up correctlyThe per-row copy handler with
copiedRowId, timeout ref, and unmount cleanup is solid: it avoids multiple pending timers and prevents React warnings about state updates after unmount. Error handling viaconsole.erroris fine for this UI context.One minor a11y improvement you could consider later: add a feature-detect guard around
navigator.clipboard(for older browsers / non-HTTPS) and expose a fallback or clearer user feedback when it’s unavailable.Also applies to: 8-15, 42-46, 47-69
133-161: Address cell rendering and tooltip behavior are correct; consider keyboard accessibilityThe new Address cell layout (full vs ellipsized address plus a copy icon with “Copied!” tooltip) integrates cleanly with
formatAddressWithEllipsis, and thecopiedRowIdstate combined with the updateduseMemodeps ensures the tooltip visibility updates as expected. Wrapping the table withstyle.transactionsTablefor responsive tweaks is also appropriate.As a follow-up improvement, you might want to:
- Swap the clickable
divfor a semantic<button type="button">or addrole="button"andtabIndex={0}plusEnter/Spacekey handling so the copy control is keyboard-accessible and the:focus-visiblestyling in CSS becomes effective.Also applies to: 164-167
components/Transaction/transaction.module.css (1)
81-91: Copy button and tooltip styling align with new UX; clean up duplicate CSS propertiesThe updated
.copy_btn, tooltip, and dark-mode rules match the new per-row copy behavior and look good.Static analysis is right about the duplicate properties though:
.copy_btnsetsdisplaytwice (inline-blockand laterflex)..tooltiptext2setsz-indextwice (1and later99999999).Since only the last declaration takes effect, consider dropping the earlier ones for clarity, e.g.:
.copy_btn { - position: relative; - display: inline-block; + position: relative; margin-left: 5px; cursor: pointer; display: flex; align-items: center; justify-content: center; flex: 0 0 auto; min-width: 20px; } .tooltiptext2 { - position: absolute; - z-index: 1; + position: absolute; width: 110px; right: 100%; /* ... */ - z-index: 99999999; + z-index: 99999999; }That will satisfy the linter and keep the intent obvious.
Also applies to: 93-102, 104-122, 124-127, 129-137, 139-142
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
components/Paybutton/PaybuttonDetail.tsx(2 hunks)components/Transaction/PaybuttonTransactions.tsx(3 hunks)components/Transaction/transaction.module.css(1 hunks)utils/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
components/Paybutton/PaybuttonDetail.tsx (1)
utils/index.ts (1)
formatAddressWithEllipsis(16-23)
components/Transaction/PaybuttonTransactions.tsx (2)
utils/index.ts (1)
formatAddressWithEllipsis(16-23)services/transactionService.ts (1)
fetchTransactionsByPaybuttonId(659-664)
🪛 Biome (2.1.2)
components/Transaction/transaction.module.css
[error] 86-86: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
display is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 118-118: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
z-index is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
⏰ 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)
utils/index.ts (1)
16-23: Centralized address ellipsis helper looks solidThe truncation logic is clear, handles short strings safely, and gives a consistent format for both bitcoin-like and other addresses. Centralizing this behavior here is a good move for reuse across components.
components/Paybutton/PaybuttonDetail.tsx (1)
12-12: Good reuse of shared address formatterUsing
formatAddressWithEllipsishere keeps Paybutton detail addresses consistent with the transactions table while still copying the full address via the icon. This is a clean refactor with no behavioral regressions.Also applies to: 61-68
components/Transaction/transaction.module.css (1)
144-153: Responsive table and dual address display classes are consistent with the React changesDefining
.transactionsTableand using it to scope the:global(.paybutton-table-ctn ...)column-hiding rules is a good way to limit the small-screen override to this table. The.addressFull/.addressShortpair with the 1599px breakpoint cleanly switch between full and truncated address views, matching how the React component renders both spans.Also applies to: 155-161, 163-170
Related to #1061
Test plan
Go to the /button/xxx page and look at the layout of the Transaction page columns. Look at it on mobile. In master, it's way worse.
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.