(IGNORE FOR NOW, will be abandonded, keeping this up for refernece). fixing nested <a> tags in transactions
AbandonedPublic
Actions

Authored by darn-it-dan on Jan 10 2022, 10:05.

Details

Reviewers

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Summary

This revision is to fix a bug where we render a nested anchor tag in our <TxHistory /> component.

Also added some types to map our behavior and wrote some tests for <TxHistory />.

My strategy to fix the bug was:
replacing the surrounding the parent <a> tag with a styled <span> tag. (the styling replicates the styling of an <a> tag).
adding a role="link" to said span tag, to make it aware to screen readers that this is a span element with "link" functionality.

I also added some tests and propTypes.

Since I am new to the project, I mapped out the types for transactions and I decided to update the PropTypes (for my benefit, and others) from my discovery reading through the code base.
My analysis of the types might be wrong so please correct me!

Test Plan

First we have to replicate the bug,
The easiest way is to make sure you have a wallet that recieved a transaction with a message.
The most obvious indicator of this is when you see a Reply To Message link in your transaction history.
Open up your console developer tools, and there should be no red error message.

Diff Detail

Repository

rABC Bitcoin ABC

Branch

fixing-nested-anchors-in-transactions

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 17849
Build 35524: Build Diff	cashtab-tests
Build 35523: arc lint + arc unit

Event Timeline

darn-it-dan created this revision.Jan 10 2022, 10:05

Owners added a reviewer: Restricted Owners Package.Jan 10 2022, 10:05

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 10 2022, 10:05

updating snapshots after linting changes

Harbormaster completed remote builds in B17849: Diff 31712.Jan 10 2022, 10:08

Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Failed tests logs:

====== CashTab Unit Tests: TxHistory Component given many transactions WITHOUT token transactions ======
Error: expect(received).toMatchSnapshot()

Snapshot name: `TxHistory Component given many transactions WITHOUT token transactions 1`

- Snapshot  - 2
+ Received  + 2

@@ -1,8 +1,8 @@
  Array [
    <span
-     class="sc-kGXeez zNAft"
+     class="sc-kGXeez inJhp"
      data-testid="txLink"
      role="link"
    >
      <div
        class="sc-kgoBCf ghiqPV"
@@ -61,11 +61,11 @@
          </span>
        </div>
      </div>
    </span>,
    <span
-     class="sc-kGXeez zNAft"
+     class="sc-kGXeez inJhp"
      data-testid="txLink"
      role="link"
    >
      <div
        class="sc-kgoBCf ghiqPV"
    at Suite.<anonymous> (/work/web/cashtab/src/components/Wallet/__tests__/TxHistory.test.js:57:29)
    at addSpecsToSuite (/work/web/cashtab/node_modules/jest-jasmine2/build/jasmine/Env.js:444:51)
    at Env.describe (/work/web/cashtab/node_modules/jest-jasmine2/build/jasmine/Env.js:414:11)
    at describe (/work/web/cashtab/node_modules/jest-jasmine2/build/jasmine/jasmineLight.js:88:18)
    at Object.<anonymous> (/work/web/cashtab/src/components/Wallet/__tests__/TxHistory.test.js:55:9)
    at processTicksAndRejections (node:internal/process/task_queues:94:5)
====== CashTab Unit Tests: TxHistory Component given many transactions WITH token transactions ======
Error: expect(received).toMatchSnapshot()

Snapshot name: `TxHistory Component given many transactions WITH token transactions 1`

- Snapshot  - 2
+ Received  + 2

@@ -1,8 +1,8 @@
  Array [
    <span
-     class="sc-kGXeez zNAft"
+     class="sc-kGXeez inJhp"
      data-testid="txLink"
      role="link"
    >
      <div
        class="sc-kgoBCf ghiqPV"
@@ -61,11 +61,11 @@
          </span>
        </div>
      </div>
    </span>,
    <span
-     class="sc-kGXeez zNAft"
+     class="sc-kGXeez inJhp"
      data-testid="txLink"
      role="link"
    >
      <div
        class="sc-kgoBCf ghiqPV"
    at Suite.<anonymous> (/work/web/cashtab/src/components/Wallet/__tests__/TxHistory.test.js:81:29)
    at addSpecsToSuite (/work/web/cashtab/node_modules/jest-jasmine2/build/jasmine/Env.js:444:51)
    at Env.describe (/work/web/cashtab/node_modules/jest-jasmine2/build/jasmine/Env.js:414:11)
    at describe (/work/web/cashtab/node_modules/jest-jasmine2/build/jasmine/jasmineLight.js:88:18)
    at Object.<anonymous> (/work/web/cashtab/src/components/Wallet/__tests__/TxHistory.test.js:79:9)
    at processTicksAndRejections (node:internal/process/task_queues:94:5)

Each failure log is accessible here:
CashTab Unit Tests: TxHistory Component given many transactions WITHOUT token transactions
CashTab Unit Tests: TxHistory Component given many transactions WITH token transactions

Harbormaster completed remote builds in B17850: Diff 31713.Jan 10 2022, 10:11

darn-it-dan edited the summary of this revision. (Show Details)Jan 10 2022, 10:16

darn-it-dan edited the summary of this revision. (Show Details)Jan 10 2022, 10:21

Good React implementations and some nice new bells and whistles for Cashtab.

Needs some work before we can get this into production.

Take a look at the ABC dev philosophy: https://github.com/Bitcoin-ABC/bitcoin-abc/blob/master/CONTRIBUTING.md#bitcoin-abc-development-philosophy

We want to keep each diff limited to a small self-contained change. In its current state, this diff is implementing a handful of things.

Changing the linting config
Fixing the <a> error
Implementing render unit tests for tx history (great add, this should be its own diff)
Improving proptypes validation in Tx.js (great add, this should be its own diff).

Please back out everything not related to "Fixing the <a> error" from this diff. But also...please submit the other changes you have worked on as their own diffs so we can ultimately pull them into the codebase.

web/cashtab/.eslintrc.js
17 ↗	(On Diff #31713)	Not sure this is related to this diff. In general, development philosophy at ABC is to change only one thing per diff. A diff should be as small as possible. It's normal to uncover issues or discover new things that should be done to the codebase while you are composing a diff. These should be logged as tasks for future diffs.
web/cashtab/src/components/Wallet/Tx.js
490 ↗	(On Diff #31713)	This is a value add to the codebase but is not related to this specific diff. Please back this out of this diff and submit it as its own diff, "Improving props validation in Tx.js"
web/cashtab/src/components/Wallet/TxHistory.js
7 ↗	(On Diff #31713)	Colors should be pulled from `src/assets/styles/theme.js` instead of being hard coded. You can see sample implementations in `App.js`

This revision now requires changes to proceed.Jan 10 2022, 23:27

darn-it-dan retitled this revision from fixing nested <a> tags in transactions to (IGNORE FOR NOW, will be abandonded, keeping this up for refernece). fixing nested <a> tags in transactions.Jan 13 2022, 18:47

Please abandon this diff -- you will still be able to reference it here

darn-it-dan abandoned this revision.Jan 19 2022, 19:24

darn-it-dan added inline comments.

web/cashtab/src/components/Wallet/TxHistory.js
7 ↗	(On Diff #31713)	This original color used for this UI is not present in theme.js. The original UI was inheriting the styling of the `<a>` tag that had a default text color of `#20242d`. is `theme.js` using some sort of format or is this a purely custom theme object? for more context: this specific color: #20242d is present in `config/webpack.config.js` which i'm using the official way of overriding the theme in `ant design`. But it seems that our `theme.js` takes priority over that? or am i assuming wrong. I'm guessing the best way to add this color is by adding it to `theme.js` like so: please let me know what the best strategy is!