[Cashtab] Upgrade Aliases screen
ClosedPublic
Actions

Authored by bytesofman on Apr 8 2024, 13:35.

Details

Reviewers

emack

Group Reviewers

Restricted Project

Commits

rABC731bffad00d4: [Cashtab] Upgrade Aliases screen

Summary

T2207

Remove antd form and modal componenets from Alias screen

Patch a bug in how refreshAliases is called in useWallet

Note: this diff reduces our bundle size by ~75kb, since we are no longer importing these components (which were not even used) with webpack

Test Plan

npm test

Diff Detail

Repository

rABC Bitcoin ABC

Branch

no-more-ei

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 28407
Build 56355: Build Diff	cashtab-tests
Build 56354: arc lint + arc unit

Event Timeline

bytesofman created this revision.Apr 8 2024, 13:35

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 8 2024, 13:35

deprecate now-unused components

Harbormaster completed remote builds in B28407: Diff 46946.Apr 8 2024, 13:38

bytesofman edited the summary of this revision. (Show Details)Apr 8 2024, 13:39

Harbormaster completed remote builds in B28408: Diff 46947.Apr 8 2024, 13:41

bytesofman published this revision for review.Apr 8 2024, 13:42

Since this diff touches on how refreshAliases is called in useWallet, can you patch the bug where right after a registration broadcast the pending alias is not immediately added to the pending aliases list, but navigating away and back to the alias screen fixes it? From a glance one of the useEffect() blocks needs to have a dependency on the aliases.pending map coming out of useWallet, so that once it is added to the pending list it can call refreshAliases.

cashtab/src/components/Alias/Alias.js
363–368 ↗	(On Diff #46947)	This simply just spits out the alias with no other context other than the green tick icon. I feel the previous "[alias] broadcasted for registration, please wait for at least 1 confirmation" was a much better message. Also need to differentiate the notification between a standard registration and one for another address. e.g. "[alias] broadcasted for registration on behalf of [address], please wait for at least 1 confirmation".
383 ↗	(On Diff #46947)
462 ↗	(On Diff #46947)	Can do without the 2nd `Warning:` text since this is in the header already. Also un-bold the body text since other alias modals only bolds the header and not the body.
473 ↗	(On Diff #46947)	Any alias registration over 13 chars will wrap and clip behind the OK/Cancel button div. Either increase modal width or adjust font for the max alias length limit.
579 ↗	(On Diff #46947)	Is it worth considering a conditional scroll bar to <Panel> if rows > 5?
cashtab/src/components/Alias/__tests__/Alias.test.js
261 ↗	(On Diff #46947)	Needs an additional test to check, post-registration broadcast, that it has been added to the pending list. This would catch the pending bug I mentioned above. Same thing for a registration confirmation update on the registered list.

This revision now requires changes to proceed.Apr 9 2024, 02:37

modal improvements

cashtab/src/components/Alias/Alias.js
363–368 ↗	(On Diff #46947)	True but imo it is time for a freeze on non-NFT based alias development. Want to avoid prematurely optimizing here.
383 ↗	(On Diff #46947)	ditto
473 ↗	(On Diff #46947)	does scrolling not show it? not ideal but difficult to get the modals "perfect" without dynamically styling the height based on length content, which imo is not the right impact ROI for cashtab. either way -- also same comment on dev freeze for non-NFT alias features.
579 ↗	(On Diff #46947)	hm probably. but this is the kind of problem where the fix is very easy --- if we discover there is a need.
cashtab/src/components/Alias/__tests__/Alias.test.js
261 ↗	(On Diff #46947)	it's a good catch but both beyond the scope of this diff and not on the dev track for NFT aliases