Related to T2291. In order to minimize reliance on external libraries, react-copy-to-clipboard has been deprecated throughout the app. However, the package remains in the package-lock.json file as it is a dependency of antd.
Details
Details
- Reviewers
bytesofman emack - Group Reviewers
Restricted Project - Commits
- rABCcd32d52d47e7: [Cashtab] Replaced all instances of react-copy-to-clipboard
cd web/cashtab && npm start
run grep -r "import { CopyToClipboard } from 'react-copy-to-clipboard';" src/ and observe that nothing is returned
from the home screen:
use the copy txid button
use the copy msg button
navigate to the receive screen
click the QR code
navigate to Send.js
generate a message signature and click on the message signature input
for each, observe that the correct data is copied to clipboard and that the success notification fires
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- deprecate-react-copy-to-clipboard
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 19289 Build 38321: Build Diff cashtab-tests Build 38320: arc lint + arc unit
Event Timeline
Comment Actions
Test plan should include grep -r "import { CopyToClipboard } from 'react-copy-to-clipboard';" src/ and confirm no instances remain
| web/cashtab/src/components/Common/QRCode.js | ||
|---|---|---|
| 8 | Adding the notification to where it was previously absent is beyond the scope of this diff. There is already a display to notify the user that clicking the QR code has copied the address to the clipboard. | |
| web/cashtab/src/components/Home/Tx.js | ||
| 690 | Since this div is repeated everywhere, make it a common component in src/components/Common.js Call it <CopyToClipbboard/> as before. | |
| web/cashtab/src/components/Common/CopyToClipboard.js | ||
|---|---|---|
| 21 ↗ | (On Diff #33899) | can improve the conditional logic. by only checking for optionalMessageHeader && optionalMessageBody inside the top level if () {} block |
| web/cashtab/src/components/Common/CopyToClipboard.js | ||
|---|---|---|
| 37 ↗ | (On Diff #33964) | Situation here where if we use the notification, both optionalMessageHeader and optionalMessageBody are required. This isn't intuitive to a would-be future dev looking at this repo. Instead, it would be better for the component to take a prop like optionalOnCopyNotification of type ` React.PropTypes.shape({
title: React.PropTypes.string,
msg: React.PropTypes.string
})You will need to handle the case of the dev only including msg or only including title by enforcing the unincluded one to be a blank string. |
Comment Actions
Responded to review feedback, PropTypes changed to object with title and msg key/value pairs. Logic has been added to the CopyToClipboard component to deal with cases where message + title & msg or title is blank.
Comment Actions
The logic in the if blocks of CopyToClipboard needs to be simplified.
It should be
- copy the text to clipboard
- if there's a notification prop, notification
| web/cashtab/src/components/Common/CopyToClipboard.js | ||
|---|---|---|
| 20 ↗ | (On Diff #34030) | This whole 'if' block should be covered by the one above it. Need to check if the parameter is included at all, i.e. !== null |