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.
Details
Details
- Reviewers
bytesofman emack - Group Reviewers
Restricted Project
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
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- move-message-signing-verfication-to-new-screen
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 20806 Build 41271: Build Diff cashtab-tests Build 41270: arc lint + arc unit
Event Timeline
Comment Actions
Great diff!
- 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
- 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.
- 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 ↗ | (On Diff #36133) | 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. |
Comment Actions
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.