Conversation
- add address text to receive modal - add copy address button to receive modal - add explorer link button to receive modal - add translation for 'explorerUnavailable'
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
Visit the preview URL for this PR (updated for commit c9def8f): https://walletrc--pull-2690-merge-3jagu22u.web.app (expires Fri, 27 Jun 2025 17:44:21 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
CharlVS
left a comment
There was a problem hiding this comment.
TYSM. Will have a closer look during office hours, but the one thing that stands out is that you should always reference theme values instead of hard-coded values since it ensures consistency/accessibility.
|
Functionality-wise, everything is working fine. However, the following observations were noted: Highlight Placement: When clicking the Copy button, the highlight appears from behind the button's bar instead of on the button itself. This looks slightly misaligned and visually inconsistent. Message Visibility: Upon clicking the Copy button, the "Copied to Clipboard" message appears at the bottom-left corner, which lies in a blurred/dark zone. It would be better if the message is displayed more prominently for better visibility. Multiple Click Behavior: If the Copy button is clicked multiple times in quick succession, the "Copied to Clipboard" message appears repeatedly even after the Receive Coin tab is closed. A suggestion would be to implement a cooldown or timer to limit repeated message display Attachment: |
AndrewDelaney
left a comment
There was a problem hiding this comment.
I ran into the same issues @ShantanuSharma9873 mentioned above. Functionally everything works well, just the UX items that should be fixed.
Additionally, on mobile screen sizes, the address gets cut off. I don't necessarilly think this is an issue, but figured it'd be best to make note of it in any case. One cannot manually scroll to view the end of the address. Then again, I don't think there are many cases where one needs to be able to do that when they can just press the copy button.
|
@smk762 please tag me when QA gives their stamp of approval. |
AndrewDelaney
left a comment
There was a problem hiding this comment.
Nice! All seems to be working well.
ShantanuSharma9873
left a comment
There was a problem hiding this comment.
The functionality works fine, nothing major. Although, one UI related issue was observed where the Address dropdown remains stuck on the screen on scrolling the page down.
Attachment:
https://github.com/user-attachments/assets/e630a5f0-00f9-4cb4-bc4c-c2eb03e913ab
Please create a new issue for this. |
takenagain
left a comment
There was a problem hiding this comment.
Thank you, @smk762! LGTM overall, apart from the one style issue in lib/shared/utils/utils.dart.
Could you (or our benevolent AI overlords) also please run dart format on the modified files? E.g. dart format lib/shared/utils/utils.dart
Thanks for the feedback! I've updated the colors, and formatted the files. AI helped with the async, but lacked the return so I added that. Ready for re-review. |
takenagain
left a comment
There was a problem hiding this comment.
LGTM, thanks! Could we remove the duplicate import before merging? Otherwise, it can be included in the clean-up of the existing flutter analyze issues at a later stage.
Co-authored-by: Francois <takenagain@users.noreply.github.com>
…e-ux/copy-address-for-merge fix(copy-address): improve clipboard overlay
There was a problem hiding this comment.
Thank you for your contribution. Congrats on your first major contribution to KW's codebase!
I've made some minor changes as discussed. Prefer using Flutter's built-in widgets/systems where possible.
My changes also make the styling for the copy confirmation consistent with other places using Flutter snackbars (most popups/toasts/snackbars are custom non-Flutter tho)
Closes #2582
🚀 New Features
Copy Address Functionality
Enhanced Copy Toast System
Copy Function Parameter Enhancement
🎨 UI/UX Improvements
Mobile Responsive Design
Professional Toast Styling
✅ QA TEST CHECKLIST
🔍 Priority 1: Critical Functionality
Copy Address Functionality
🔍 Priority 2: UI/UX Validation
Address Text Display
0x1234...abcdnot0x1234567...QR Code Dialogs
Toast/Snackbar Behavior
🔍 Priority 3: Edge Cases
Long Addresses
Error Scenarios
Performance
🔍 Priority 4: Regression Testing
Existing Functionality
Address Management
🚨 Known Issues / Considerations
📊 Success Criteria