Page MenuHomePhabricator

[Cashtab] Move message signing and verification to new screen
AbandonedPublic

Authored by kieran709 on Nov 4 2022, 16:34.

Details

Reviewers
bytesofman
emack
Group Reviewers
Restricted Project
Summary

Related to T2734. Message signing and verification Collapses have been moved to their own page. A new component and route have been added, a new NavItem has been added to the hamburger menu, and snapshot testing has been implemented following the Send and SendToken template. This is the first time I have initialized a component with snapshots so I am expecting to make significant changes to SignVerigyMsg.test.js.

Test Plan

cd web/cashtab && npm start
expand the hamburger menu
navigate to Sign & Verify screen
ensure sign and verify UI and functionality is working

navigate to send screen
observe that the Sign and Verify Collapses are no longer present

npm test
observe that all tests pass

Diff Detail

Repository
rABC Bitcoin ABC
Branch
move-message-signing-verfication-to-new-screen
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 20807
Build 41273: Build Diffcashtab-tests
Build 41272: arc lint + arc unit

Event Timeline

bytesofman requested changes to this revision.Nov 5 2022, 05:04

Great diff!

  1. See inline comment. I don't think App.js should be modified to pass these params to the new component, when other child components are not receiving these params from App.js
  2. Need some styling work in the hamburger menu. The names should be centered in the left column. Sign and Verify length creates too much empty space to the left of other options.
  3. This is a good working proof of concept diff. However, there is a lot going on here. This should be a stacked diff. Think about how you could divided this problem into smaller sub tasks and submit each one as part of a stack.

For example, something like

1 - add new icon
2 - remove from send.js
3 - add to new screen
4 - add snapshot tests

web/cashtab/src/components/SignVerifyMsg/SignVerifyMsg.js
75

It's probably worth studying what all should be pulled from context in App.js and then passed down to child components. Maybe getting settings in App.js and passing it down is best.

However, none of the other screens do that right now. So, this one shouldn't either unless there is some reason unique to this new component where this is required.

This revision now requires changes to proceed.Nov 5 2022, 05:04

To keep it clean I am going to abandon this diff and begin the process of pushing it up stacked as mentioned in the review.