Page MenuHomePhabricator

[Cashtab] Created TokenIcon component
AbandonedPublic

Authored by kieran709 on Jan 10 2022, 16:04.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Summary

Created TokenIcon component and added it throughout the app. I am having trouble getting the Popover in SendToken.js to render the component, but I can continue to work on it.

Test Plan

cd web/cashtab
npm start
Ensure that there are token txs within recent history
Check that custom token icons and generated icons are appearing
Naviagate to eTokens tab
Check that custom token icons and generated icons are appearing
Navigate to SentToken page
Check that custom icons and generated icons are appearing in the amount field of the send form
Currently, the popover icon in the SendToken page is not rendering the component, which is why it not using the TokenIcon component. I am aiming to fix this issue.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
token-icon-component
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 17865
Build 35556: Build Diffcashtab-tests
Build 35555: arc lint + arc unit

Event Timeline

Still working on rendering the component in the SendToken popover.

Cleaned up antd popover to include the TokenIcon component.

bytesofman added inline comments.
web/cashtab/src/components/Tokens/TokenIcon.js
8 ↗(On Diff #31793)

Is this empty div necessary? Seems like <Wrapper> is still appearing in the files hosting the new <TokenIcon> component

This revision now requires changes to proceed.Jan 13 2022, 23:07

Replaced empty wrapper div with a fragment component.

bytesofman added inline comments.
web/cashtab/src/components/Send/SendToken.js
371 ↗(On Diff #31834)

Does this <span> have to be here?

web/cashtab/src/components/Tokens/TokenIcon.js
11 ↗(On Diff #31834)

It's okay to assume that this will never be the case going forward. This was a placeholder from when we didn't have the icon backend up.

So, remove this conditional logic. Component can just be the Img with unloader.

This revision now requires changes to proceed.Jan 20 2022, 16:48

Responding to review feedback, removed conditional logic from TokenIcon component. Initially I thought the span wrapping the component was necessary for displaying it inside the popover, but it now appears to be working fine without the span. Additionally, updated snapshots.

bytesofman added inline comments.
web/cashtab/src/components/Send/SendToken.js
379 ↗(On Diff #31876)

Do we need this <span>?

This revision now requires changes to proceed.Jan 20 2022, 22:11

Rebased, resolved conflicts, updated snapshots.

web/cashtab/src/components/Send/SendToken.js
379 ↗(On Diff #31876)

Without a parent element the popover will not render with the TokenIcon component inside.

image.png (384×550 px, 135 KB)

I'm seeing this ok after removing the <span> tags

This revision now requires changes to proceed.Jan 24 2022, 21:04

responding to review feedback, removed span that wrapped TokenIconComponent in SendToken component.

bytesofman added inline comments.
web/cashtab/src/components/Send/SendToken.js
387 ↗(On Diff #32064)

why is token.tokenId used here but former code (and 2 lines below) uses tokenId?

If tokenId is available, just use that everywhere.

web/cashtab/src/components/Tokens/TokenIcon.js
32 ↗(On Diff #32064)

Must be 32, 64, 128, 256, or 512 -- not just any arbitrary number

size: PropTypes.oneOf([32, 64, 128, 256, 512]),

This revision now requires changes to proceed.Jan 31 2022, 21:26

Responding to review feedback

This is good to go, but we'll put this in after the UI overhaul

Rebased and corrected conflicts.

Local repo pushed files that have been deleted since the UI overhaul, abandoning and re-submitting.