Page MenuHomePhabricator

[Cashtab][Alias] Check Alias function
ClosedPublic

Authored by emack on Oct 31 2023, 13:52.

Details

Reviewers
bytesofman
Fabien
Group Reviewers
Restricted Project
Commits
rABC6954c1d261ed: [Cashtab][Alias] Check Alias function
Summary

Simplified and more efficient version of D14666 where a Check Alias button is added which leverages the existing registration input field and validation logic.
Results from the check are rendered via UI notifications.

Clicking Check Alias on an alias with pending registrations

image.png (387×599 px, 90 KB)

Clicking Check Alias on an alias with no pending registrations

image.png (408×599 px, 94 KB)

When an invalid alias is entered, the Check Alias button is also disabled

image.png (398×547 px, 40 KB)

Test Plan

npm test
npm start
Check on a registered, unregistered and pending alias and observe the UI notifications on their status
Input an invalid alias and ensure the Check Alias button is disabled

Diff Detail

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

Event Timeline

emack requested review of this revision.Oct 31 2023, 13:52
Fabien requested changes to this revision.Oct 31 2023, 15:02

UI remark: the ordering of the elements is not the best imo. Here is what I would do:
Input field
Alias length
Check button
Register using the wallet address checkbox
Register button

This follows the workflow: first check, then register. It also prevents clicking the wrong button by mistake as they are more separated.

A few more UI nits:
Register active wallet address => Register to the active wallet address
The register button could show the registration fee, like "Register alias for 556 XEC".
If this doesn't fit due to space constaints, the fee could be added to the length string "This alias is 8 bytes in length, registration fee is 556 XEC"

This revision now requires changes to proceed.Oct 31 2023, 15:02
emack edited the summary of this revision. (Show Details)

Re-arranged UI per feedback, updated label for active wallet registration checkbox.

The register button could show the registration fee, like "Register alias for 556 XEC".

This was a design decision to not show the price here because it would be an API call per keystroke to get the price for current length. Even though the price is not expected to change frequently we felt it was better to have a single source of truth on the server side, which is called only once at the registration confirmation dialogue. Alternatively I can keep a copy of the pricing array in Cashtab so it can achieve pricing reads on the fly without calling the API but I'll need to manually keep it in sync with any pricing changes on the server.

Rejigged UI for reference:

image.png (558×844 px, 100 KB)

bytesofman requested changes to this revision.Nov 1 2023, 17:57
  • add some space between the alias length and the check button
  • the check and register buttons should not look identical. maybe include a magnifying glass ("search") icon on the check button, and a dollar sign or some other appropriate icon on the register button. Also different colors.
This revision now requires changes to proceed.Nov 1 2023, 17:57

Updated UI per feedback.

image.png (395×575 px, 41 KB)

Also note the snapshot test updates to unrelated components is due to the updates to src/assets/styles/theme.js from this diff which sits globally across the app.

Fabien requested changes to this revision.Nov 2 2023, 11:08

Re-arranged UI per feedback, updated label for active wallet registration checkbox.

The register button could show the registration fee, like "Register alias for 556 XEC".

This was a design decision to not show the price here because it would be an API call per keystroke to get the price for current length. Even though the price is not expected to change frequently we felt it was better to have a single source of truth on the server side, which is called only once at the registration confirmation dialogue. Alternatively I can keep a copy of the pricing array in Cashtab so it can achieve pricing reads on the fly without calling the API but I'll need to manually keep it in sync with any pricing changes on the server.

I thought this was done already ? Or maybe it's only on server side, I can't remember.
You should do an API call on each block and store the value locally, obviously a lookup for each keystroke is the wrong design.

This revision now requires changes to proceed.Nov 2 2023, 11:08

I don't see the registration fee in the snapshots ? Otherwise the last one LGTM

nice, buttons look way better now

You should do an API call on each block and store the value locally, obviously a lookup for each keystroke is the wrong design.

this is a good idea but imo should be it's own diff. not necessarily related to just this UI change, easy enough to add into this UI -- but does require some new logic.

emack requested review of this revision.Nov 2 2023, 21:59

You should do an API call on each block and store the value locally, obviously a lookup for each keystroke is the wrong design.

this is a good idea but imo should be it's own diff. not necessarily related to just this UI change, easy enough to add into this UI -- but does require some new logic.

@Fabien - ^ price sync being implemented via D14728 and once landed I'll do a separate diff to add in localized price displays for alias input.

This revision is now accepted and ready to land.Nov 3 2023, 13:47
This revision was automatically updated to reflect the committed changes.