Page MenuHomePhabricator

[Cashtab] Update SignVerifyMsg with direct calls to antd notifications
ClosedPublic

Authored by emack on Jan 28 2024, 10:58.

Details

Summary

This is a precursor to fully deprecating Notifications.js.

Previous efforts to consolidate all notification permutations into a single function have yielded little efficiency compared to simply straight-up using the antd component as per this diff.

Having said that, directly calling the antd notification component also introduces challenges in inserting the data-testid tags needed for automated integration tests to work. However this was mitigated by using the queryByText API to locate the resulting notification for test verification.

Test Plan
  • npm test
  • grep -r messageSignedNotification src/ with no output
  • grep -r MessageSignedNotificationIcon src/ with no output
  • npm start
  • Navigate to the Sign & Verify component and trigger the following notifications:
  • Signature generation success

before

image.png (144×404 px, 13 KB)
after
image.png (146×406 px, 18 KB)

  • Signature verification success (no change to UI)
  • Signature verification failure (no change to UI)
  • load d15305.netlify.app on mobile device and verify the above
  • npm run extension and verify the above on extension

Diff Detail

Repository
rABC Bitcoin ABC
Branch
pureSignVerifyMsgNotif
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 26701
Build 52970: Build Diffcashtab-tests
Build 52969: arc lint + arc unit

Event Timeline

Added automated integration tests and supporting testid tags

Removed unused modal tags

emack published this revision for review.Jan 28 2024, 13:18
emack added a reviewer: bytesofman.

good integration tests overall, just looking for one other case

cashtab/src/components/SignVerifyMsg/__tests__/SignVerifyMsg.test.js
66

please include a test showing this is NOT rendered before signing

sometimes these components are in the DOM and not displayed

This revision now requires changes to proceed.Jan 28 2024, 14:26
emack marked an inline comment as done.

Added an additional test showing notification is NOT rendered before signing/verification

Also version bump.

  • Either include <Paragraph> component in all the notifications or none of the notifications (preference for none since simpler, unless that is really ugly for some reason)
  • We should have the same css mobile rule for all of these notifications, not a function being passed every time
cashtab/package.json
3 ↗(On Diff #44664)

1.0.9

cashtab/src/components/Common/Notifications.js
13–14 ↗(On Diff #44664)

this strikes me as a stupid function. we are using the same antd component for everything. so, we should just have a rule based on device width.

apply mobile rules for any width < 768px across the board. Stop passing style in calls to antd notification

is this the only thing we are using react-device-detect for? If so we should get rid of that lib.

unrelated to this diff but should be fixed before going through the rest of the notification improvements, so might as well do it before this diff so we don't have to back out importing this to SignVerifyMsg

cashtab/src/components/SignVerifyMsg/SignVerifyMsg.js
126 ↗(On Diff #44664)

Let's put this in a <Paragraph> as well (or put nothing in <Paragraph> -- what does the <Paragraph> do for the apperance?)

we're not really looking to just "do exactly what we used to do" in these diffs -- we want to make it better

This revision now requires changes to proceed.Jan 29 2024, 17:47
emack marked 2 inline comments as done.

Rebased to D15338 and D15323 which deprecated the react-device-detect library, deprecated the getDeviceNotificationStyle function, as well as removed the <Paragraph> tags and passing in of style props into the antd notification calls.

Also got rid of the message signing custom icon, which was just the eToken logo, which in turn, was just the XEC logo. Removing this icon prop reverts to the default tick/cross icons, making it more consistent.

d15305.netlify.app also updated

rebase and version bump from latest master

This revision is now accepted and ready to land.Jan 31 2024, 19:37

Rebase and version bump