Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABCe53318a95b5d: [Cashtab] Update SignVerifyMsg with direct calls to antd notifications

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

after

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 26699
Build 52966: Build Diff	cashtab-tests
Build 52965: arc lint + arc unit

Event Timeline

emack created this revision.Jan 28 2024, 10:58

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 28 2024, 10:58

Harbormaster completed remote builds in B26699: Diff 44659.Jan 28 2024, 11:01

emack edited the summary of this revision. (Show Details)Jan 28 2024, 11:41

Added automated integration tests and supporting testid tags

Harbormaster completed remote builds in B26700: Diff 44660.Jan 28 2024, 12:12

Removed unused modal tags

emack edited the test plan for this revision. (Show Details)Jan 28 2024, 12:21

Harbormaster completed remote builds in B26701: Diff 44661.Jan 28 2024, 12:22

emack edited the test plan for this revision. (Show Details)Jan 28 2024, 13:14

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

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

Also version bump.

Harbormaster completed remote builds in B26704: Diff 44664.Jan 29 2024, 01:25

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 edited the test plan for this revision. (Show Details)Jan 31 2024, 12:29

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

Harbormaster completed remote builds in B26796: Diff 44791.Jan 31 2024, 12:55

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

Harbormaster completed remote builds in B26815: Diff 44816.Jan 31 2024, 23:25

version bump

Harbormaster completed remote builds in B26819: Diff 44823.Feb 1 2024, 00:22

Closed by commit rABCe53318a95b5d: [Cashtab] Update SignVerifyMsg with direct calls to antd notifications (authored by emack). · Explain WhyFeb 1 2024, 00:24

This revision was automatically updated to reflect the committed changes.

emack added a commit: rABCe53318a95b5d: [Cashtab] Update SignVerifyMsg with direct calls to antd notifications.