- 
                Notifications
    
You must be signed in to change notification settings  - Fork 208
 
feat(qrcode): add hover-to-copy functionality #2851 #3345
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
base: master
Are you sure you want to change the base?
Conversation
| 
           Hi @reneaaron  @stackingsaunter  | 
    
        
          
                package.json
              
                Outdated
          
        
      | "prepare": "husky install" | ||
| }, | ||
| "dependencies": { | ||
| "@bitcoin-design/bitcoin-icons-react": "^0.1.10", | 
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.
why new library for just one icon. we use popicons by default. and i guess copy icon should be there as well PopiconsCopyLine
        
          
                src/app/components/QRCode/index.tsx
              
                Outdated
          
        
      | import { classNames, useTheme } from "~/app/utils"; | ||
| import { useState } from "react"; | ||
| import { useTranslation } from "react-i18next"; | ||
| import { CopyIcon } from "@bitcoin-design/bitcoin-icons-react/filled"; | 
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.
use popicons
        
          
                src/app/screens/Receive/index.tsx
              
                Outdated
          
        
      | value={lightningAddress} | ||
| size={192} | ||
| level="Q" | ||
| onCopy={() => toast.success("Copied to clipboard")} | 
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.
why this new callback was introduced? just to show success message? can't we do same within the qr code component?
        
          
                src/i18n/locales/en/translation.json
              
                Outdated
          
        
      | }, | ||
| "website_permissions": "Website Permissions" | ||
| "website_permissions": "Website Permissions", | ||
| "qrcode": { | 
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.
we already have copy_clipboard and copied_to_clipboard in common section of translations. always check before introducing new ones
        
          
                src/app/components/QRCode/index.tsx
              
                Outdated
          
        
      | bgColor={bgColor} | ||
| className={classNames( | ||
| "w-full h-auto rounded-md transition-opacity", | ||
| isHovering ? "opacity-80" : "opacity-100", | 
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.
for hover why we need to use state varbiles? can use tailwind here only?
eg.
className="text-gray-600 hover: text-gray-800"
| 
           screenshot looks a bit messsy. missing translations broken ui. can you check if other elements are not effected by your ui change and update screenshots? thanks!  | 
    
| 
           Thanks for your review, @pavanjoshi914 .I’ll make the suggested changes and update the PR shortly.  | 
    
…de and @popicons/react
| level={level} | ||
| /> | ||
| <div className="absolute inset-0 flex items-center justify-center opacity-0 group-hover:opacity-100 transition-opacity"> | ||
| <PopiconsCopyLine className="h-8 w-8 text-white drop-shadow-lg" /> | 
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.
where this is used now? i don't see any apperance of this
| <QRCode | ||
| value={invoice.paymentRequest.toUpperCase()} | ||
| size={512} | ||
| size={192} | 
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.
revert
| 
           
 @stackingsaunter can you please assit him in hover opacity and overlay that should be shown when user hovers the qr code. currently its not that clear visually that we can copy qr code  | 
    
| 
           @pavanjoshi914 , I will make sure the required changes are done!!  | 
    
| 
          
 I've made the following changes based on your feedback: Re-added the overlay with: <div className="absolute inset-0 bg-black bg-opacity-50 flex items-center justify-center opacity-0 group-hover:opacity-100 transition-opacity z-20">Reverted the QR code size back to 512: <QRCode value={invoice.paymentRequest.toUpperCase()} size={512} />I tried implementing the copy functionality on the icon but haven't been able to get it working yet. Still exploring possible approaches—if you have any suggestions, I’d love to hear them! The above two changes haven't been pushed yet — I’ll push them together once the copy feature on the logo is working. Let me know if there's anything else you'd like me to check meanwhile. Thanks again for your review!  | 
    
| 
           @pavanjoshi914 any suggestions?  | 
    
| 
           @stackingsaunter any suggestions from you side to solve the issue?  | 
    



Describe the changes you have made in this PR
Added a hover-to-copy interaction on the QRCode component. When the user hovers over the QR code, a semi-transparent overlay appears with a copy icon and text prompting the user to click. On click, the QR code content is copied to the clipboard. This enhances UX by making the copy functionality more discoverable and user-friendly.
Link this PR to an issue
Fixes #2851
Type of change
feat: New feature (non-breaking change which adds functionality)Screenshots of the changes [optional]
How has this been tested?
Checklist