Differential D17996

[cashtab] Transaction row UI
ClosedPublic
Actions

Authored by johnkuney on Fri, Apr 25, 13:12.

Details

Reviewers

bytesofman

Group Reviewers

Restricted Project

Commits

rABCd40c68b72040: [cashtab] Transaction row UI

Summary

Proposed ui updates to the transaction rows on the home view.
Basically some general tweaks to improve readability, make it look less cluttered, and help visually pair the transaction with
the transaction info (ie messages and token info etc)

Test Plan

preview and check the UI looks nice in different scenarios

before

after

Overall subtle changes, but the things to notice

added a background to the row so each tx is visually contained and we can lose all the lines
removed copy icon next to amount. Functoinality remains but removes the clutter as most the time this is unused
improved castab messages display. before shorter messages would display on the right
add background circle to icons

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

johnkuney created this revision.Fri, Apr 25, 13:12

Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, Apr 25, 13:12

johnkuney requested review of this revision.Fri, Apr 25, 13:12

remove comment

Harbormaster completed remote builds in B33107: Diff 53694.Fri, Apr 25, 13:17

Harbormaster completed remote builds in B33108: Diff 53695.Fri, Apr 25, 13:20

review is simplified in this type of diff if before + after screenshots highlighting what has changed are included in the test plan

does seem to be better but it's hard to work out exactly what's changing

cashtab/src/assets/styles/theme.ts
46 ↗	(On Diff #53695)	this isn't really changing anything right? can back it out
cashtab/src/components/Home/Tx/index.tsx
1020 ↗	(On Diff #53695)	what are we achieving here by losing the component and using this custom div and function? if it's a good change, why not implement it in CopyIconButton? I'm not saying we need to do that, it's just unclear from this diff why that approach is avoided.

This revision now requires changes to proceed.Fri, Apr 25, 13:51

Ah sure good point, will add screen caps and more description to test plan

cashtab/src/assets/styles/theme.ts
46 ↗	(On Diff #53695)	yes it may appear so, but I'm using this color value in a few places in this diff but with a hex alpha applied, and the hex alpha wont work with the shorthand hex value
cashtab/src/components/Home/Tx/index.tsx
1020 ↗	(On Diff #53695)	well I was trying to reduce clutter so looking to axe the copy icon on every amount. Instead just have it copy the amount if the number is clicked. So it doesnt really fit into the copyiconButton component anymore. Could potentially add a prop to copyiconButton to render text instead of the icon to use here, but I dont think thats better

@bot preview-cashtab

teamcity awarded a token.Fri, Apr 25, 14:17

johnkuney edited the test plan for this revision. (Show Details)Fri, Apr 25, 14:19

Build Bitcoin ABC Diffs / Diff Testing (preview-cashtab) passed.
Preview is available at http://51.68.37.192:41375 for the next 60 minutes.

johnkuney edited the test plan for this revision. (Show Details)Fri, Apr 25, 14:22

Overall I like it, especially the lines removal. But the margin is way more than before and now on my (big) screen I can three 3 full txs vs 5 before the change. Can we remove some of these margins ? Especially the vertical rows spacing inside the tx block

reduce margins

for sure, reduced them a bit

Harbormaster completed remote builds in B33109: Diff 53696.Fri, Apr 25, 14:48

bytesofman added inline comments.Fri, Apr 25, 16:39