Page MenuHomePhabricator

[Cashtab] Upgrade Aliases screen
ClosedPublic

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

image.png (112×603 px, 15 KB)

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

deprecate now-unused components

bytesofman published this revision for review.Apr 8 2024, 13:42
emack requested changes to this revision.Apr 9 2024, 02:37
emack added a subscriber: emack.

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".

image.png (103×358 px, 8 KB)

383 ↗(On Diff #46947)
462 ↗(On Diff #46947)

Can do without the 2nd Warning: text since this is in the header already.

image.png (137×356 px, 21 KB)

Also un-bold the body text since other alias modals only bolds the header and not the body.

image.png (168×328 px, 29 KB)

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.

image.png (396×519 px, 62 KB)

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
bytesofman marked 5 inline comments as done.

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

This revision is now accepted and ready to land.Apr 9 2024, 03:24
This revision was automatically updated to reflect the committed changes.