feat: redesign send rpc transfer flow

[alter-eggo]

Try out Leather build f56dbcd — Extension build, Test report, Storybook, Chromatic

redesign rpc send transfer using approver ux

[kyranjamie]

Great work @alter-eggo, leaving some comments.

I'll merge my PR now, which has some of the changes to focusTab helper you can use here

[kyranjamie]

Shouldn't these presentation/formatting concerns be directly in component?

[alter-eggo]

we will use it in other bitcoin flows as well, so prefer keep in this file, though took it out of fees hook

[pete-watters]

Maybe you can use a .layout file to help split this up between logic and presentation?

[alter-eggo]

don't understand how to split tbh and why need? what should be in .layout?

[kyranjamie]

There are components with many style props, can't they be in a layout component?

In getAddresses the entire Approver component is in the layout, such that the root RPC compoent can be focused on business logic/event passing behaviours, and not layout and styles.

[alter-eggo]

looking at this file I believe main problem here is with huge approve logic. I will try to move this to sep file.
actually we'll need this in other bitcoin related flows as well

[alter-eggo]

same with selected fee

[alter-eggo]

and prob then will be way easier to move it to layout file. will try

[pete-watters]

Great work here @alter-eggo 🚀 huge PR!

I added a few suggestions but looking good to me overall

[fbwoolf]

Nice work @alter-eggo! I migrated some of the bitcoin fees code to the mono repo for mobile. I have an issue open to install it back into the extension to make sure it is shared, but that can come later.

[kyranjamie]

Building on earlier comment, why don't we do the capitalize in the view too? How we format/present data is a concern of the UI so better lives where it's set to a component.

My thinking is that if I as a dev need to go and make visual changes, they can be mostly scoped to a set of .layout components. But in this file, we have a mix of mathematics and formatting, that I can't help but feel should live separately.

e.g. <style.span>{capitalize(type)}</styled.span>

Then, the utils file knows nothing about where (titleLeft), or how (capitalised), the data needs to be displayed, focusing on accurate data only.

What's your thinking for putting it here, and are there reasons I'm missing why it's beneficial to group it here?

[alter-eggo]

I tried to do FeeItem as dumb as possible, so general idea here was to gen and pass ready to use parts of the fee component, title/caption left/right

yeah, maybe better to capitalize in component itself, and so remove titleLeft, I just thought it would be better to keep all these parts in one place.

so in stx I can do the same - get raw fee => then gen ready to use parts of fee component

Then, the utils file knows nothing about where (titleLeft), or how (capitalised), the data needs to be displayed, focusing on accurate data only.

if not utils so what's the best place to keep it? other way btw - create bitcoin fee wrapper for shared fee item and keep this logic there

[markmhendrickson]

Looking good! Some QA feedback:

• App favicon isn't showing up in the header
• "Total spend" doesn't include the fee amount
• Guide URL takes user to https://leather.io/guides/connect-dapps instead of a relevant guide for sending BTC via an app
• Do we want to say "Send bitcoin" instead of "Send BTC"? cc @fabric-8 @mica000
• I'd suggest some additional design QA (by a designer) to double check text styling / sizes, padding, etc.
• Header for edit fee screen has unexpected top and bottom padding in beige

[fabric-8]

Looking great, only some minor feedback:

• animal icon stroke width are too thin, should generally be same as for the other icons. Maybe they are getting scaled because there's a mismatch of height/width it needs (32x32) vs. actual size after some... padding is applied? If I can help by adjusting the asset lmk, the animation icons are 32x32 vs. 24x24, if turning them into 24x24 fixes this issue I can do it

• Since were are using the > to indicate that the user can interact with the item (like for account on top), I think we should also add the > to fee settings for consistency cc @mica000

• Realising that "cancel" in fee settings might get misinterpreted to "cancel the whole approval thing" - Maybe using the word "back" would be better here? cc @markmhendrickson

• Let's add border-top: 1px solid ink.border-default to the footer

[mica000]

This is looking great @alter-eggo! Really nice to see it coming together! Bravo 👏

[mica000]

Are we covering advanced details as well on this PR? And is this only for BTC or for STX as well?
If so, here are the details we would need to trigger for Asset transfer BTC and AssetTransferSTX

[mica000]

@fabric-8 @markmhendrickson Curious about what you think of the prompt, as it shouldn't show the icon of the application since we are already showing this on the request by. However, it is quite nice to have some reassuring of the app you are connecting to:

Here is how it was intended to work and the respective designs

[mica000]

@markmhendrickson @fabric-8 We might not have explicitly specified this in the approval documentation, but when connecting to a BTC account, the total balance should be calculated solely for BTC. Including other assets in the calculation could confuse users, especially if they meet the required balance but not in BTC. Thoughts? And if so, should this be in this PR or a later improvment?

[mica000]

Account selection: When selecting an account does it make sense to show create account cta? Maybe better to hide it?

[mica000]

Looking great, only some minor feedback:

• animal icon stroke width are too thin, should generally be same as for the other icons. Maybe they are getting scaled because there's a mismatch of height/width it needs (32x32) vs. actual size after some... padding is applied? If I can help by adjusting the asset lmk, the animation icons are 32x32 vs. 24x24, if turning them into 24x24 fixes this issue I can do it

• Since were are using the > to indicate that the user can interact with the item (like for account on top), I think we should also add the > to fee settings for consistency cc @mica000

• Realising that "cancel" in fee settings might get misinterpreted to "cancel the whole approval thing" - Maybe using the word "back" would be better here? cc @markmhendrickson
• Let's add border-top: 1px solid ink.border-default to the footer

Yes for adding > to fee settings for consistency

[markmhendrickson]

Realising that "cancel" in fee settings might get misinterpreted to "cancel the whole approval thing" - Maybe using the word "back" would be better here? cc @markmhendrickson

Agreed

Curious about what you think of the prompt, as it shouldn't show the icon of the application since we are already showing this on the request by. However, it is quite nice to have some reassuring of the app you are connecting to:

I do like the idea of including the app icon, though it takes up a lot of space in the current prompt design. It seems perhaps best to just rely on this placement so we don't push the other content below the fold?

We might not have explicitly specified this in the approval documentation, but when connecting to a BTC account, the total balance should be calculated solely for BTC. Including other assets in the calculation could confuse users, especially if they meet the required balance but not in BTC. Thoughts? And if so, should this be in this PR or a later improvment?

It seems best that we either 1) show just a fiat value (for the total account value including all assets, as here in the PR currently) or 2) show the fiat value for the specific asset in question (e.g. BTC value in USD here) and that asset's literal value (e.g. BTC balance itself here) right below it, as in your designs. By placing these two values together vertically, it'll presumably be clear to the user just which subset of value they're looking at it in this context.

We could add this enhancement (2) later or tackle as part of this PR – both work for me.

[alter-eggo]

• removed app icon
• fixed icons
• added chevron in fees item and border in footer actions
• changed cancel to back

@mica000 re: create new acc btn - I'd prefer to do it in sep pr since will lead to some refactoring, which I don't want to do here, since pr is already quite big

[mica000]

Thanks @alter-eggo!

Icons for fees remains 1 px stroke?

[mica000]

@alter-eggo About the account picker cta, no worries!

In relation to the component on the approval however, lets do the second option suggested by @markmhendrickson in this comment?

2. show the fiat value for the specific asset in question (e.g. BTC value in USD here) and that asset's literal value (e.g. BTC balance itself here) right below it, as in your designs. By placing these two values together vertically, it'll presumably be clear to the user just which subset of value they're looking at it in this context.

In add, @markmhendrickson I believe we can hide one of the address, depending on the network being used.
so it would look more like this:

[alter-eggo]

@mica000 done

[mica000]

@alter-eggo thanks! Looking good!

[markmhendrickson]

@alter-eggo it appears this change is still needed given my latest testing:

Guide URL takes user to https://leather.io/guides/connect-dapps instead of a relevant guide for sending BTC via an app