Page MenuHomePhabricator

[Cashtab] Replaced all instances of react-copy-to-clipboard
ClosedPublic

Authored by kieran709 on Jun 7 2022, 16:46.

Details

Summary

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.

Test Plan

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

Repository
rABC Bitcoin ABC
Branch
deprecate-react-copy-to-clipboard
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 19310
Build 38360: Build Diffcashtab-tests
Build 38359: arc lint + arc unit

Event Timeline

bytesofman requested changes to this revision.Jun 7 2022, 17:34

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 ↗(On Diff #33864)

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 ↗(On Diff #33864)

Since this div is repeated everywhere, make it a common component in src/components/Common.js

Call it <CopyToClipbboard/> as before.

This revision now requires changes to proceed.Jun 7 2022, 17:34

responding to review feedback

bytesofman requested changes to this revision.Jun 9 2022, 16:11
bytesofman added inline comments.
web/cashtab/src/components/Common/CopyToClipboard.js
21

can improve the conditional logic. by only checking for optionalMessageHeader && optionalMessageBody inside the top level if () {} block

This revision now requires changes to proceed.Jun 9 2022, 16:11

responding to review feedback

bytesofman added inline comments.
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.

This revision now requires changes to proceed.Jun 13 2022, 18:13

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.

The logic in the if blocks of CopyToClipboard needs to be simplified.

It should be

  1. copy the text to clipboard
  2. 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

This revision now requires changes to proceed.Jun 17 2022, 17:11

Refactored onClick function

This revision is now accepted and ready to land.Jun 23 2022, 22:37