Create transaction screen and rework addresses screen #38

nop33 · 2022-08-18T11:59:54Z

No description provided.

src/components/IOList.tsx

src/components/buttons/Button.tsx

src/components/buttons/ButtonsRow.tsx

src/components/headers/DefaultHeader.tsx

src/screens/TransactionScreen.tsx

src/components/buttons/Button.tsx

src/components/AddressBadge.tsx

LeeAlephium · 2022-08-18T14:46:04Z

Functional test looks good, couple comments:

"Fee" in the transaction screen when in "Discreet mode" is blocked out. I don't understand how that's useful.
No user labels are used on the Transfers screen.

LeeAlephium · 2022-08-18T15:37:52Z

Please do not change the context of the PR to include more than it needs to - please stack this PR with another to address changes to the addresses screen - there were other things to address in this PR before adding more to it

nop33 · 2022-08-22T07:32:21Z

Please do not change the context of the PR to include more than it needs to - please stack this PR with another to address changes to the addresses screen - there were other things to address in this PR before adding more to it

I was not planning on exceeding the 1000 LOC (unofficial) limit. I did not rewrite history, so you can review the new changes I added separately from the previous ones. I can address all the review comments together. Does this create a problem for you?

nop33 · 2022-08-22T07:44:16Z

"Fee" in the transaction screen when in "Discreet mode" is blocked out. I don't understand how that's useful.

It's a configurable amount by the user, so we hide it. I don't see a reason not to, same as all other amounts. Do you have a strong argument against it?

No user labels are used on the Transfers screen.

What do you mean? As in "input field labels"?

LeeAlephium · 2022-08-22T12:03:15Z

Does this create a problem for you?

Yes, the scope of the PR is growing making it more complicated

Do you have a strong argument against it?

I don't, but I also don't understand why it's useful to hide a fee amount which I'd like to. The purpose of a fee is to pay for network resources so what is sensitive about this?

What do you mean? As in "input field labels"?

I'll get you a picture, I don't know how else to describe it other than all the user owned addresses with labels aren't using their label on that view

mvaivre · 2022-08-22T12:15:08Z

My little grain of salt regarding hiding the amounts : IMO, the only amounts that should be hidden are the holdings & read-only values (tx values in history for instance).
Any writable / editable amount, or any amount that isn't linked to an hodled amount shouldn't be hidden in order to allow for transparency during manual operations. Hence IMHO the fees should indeed stay visible.

nop33 · 2022-08-22T12:42:02Z

I don't fully get the argument for the fees being visible in discreet mode, but since both of you support that I'll change it 👍

nop33 · 2022-08-22T12:51:40Z

Yes, the scope of the PR is growing making it more complicated

It did grow, but won't grow more than this 😇. It's 800 LOC and I think it's reasonable. I don't think I did something wrong, I asked for your review, you gave it, I pushed some new code ensuring it won't grow much, I asked for it again. Is that something we don't want to do in the future? We agreed that we shouldn't rewrite history so that people can review newly added commits. Which I respected. Would you have said the same it I only asked for your review once after pushing the new code? If you don't feel like reviewing it, I can pass it to @mvaivre. I won't split it, it creates overhead.

nop33 · 2022-08-22T12:52:45Z

No user labels are used on the Transfers screen.

What do you mean? As in "input field labels"?

I'll get you a picture, I don't know how else to describe it other than all the user owned addresses with labels aren't using their label on that view

Ah, you mean address labels. The Transfers screen is not in the scope of this PR. It's also not properly represented in the mockups.

nop33 · 2022-08-22T13:20:05Z

I am changing the status of this PR to WIP. I cannot mark it as "Draft" otherwise, and I cannot remove reviewers either.

mvaivre · 2022-08-22T14:31:00Z

It's also not properly represented in the mockups.

As of today, I actually think that specifying the addresses' labels in the "global" TX list view is not a vital information. I'd like to start without it, try the UI, and iterate if necessary :)

LeeAlephium

Damn that carousel is nice to have ✨ Code looks good!

On the functional test I noticed one thing in particular: if you press on an address card and go back, the copy and qr code buttons remain disabled.

Screen_Recording_20220822-073107_mobile-wallet.mp4

Two minor things I noticed: it's not obvious when pressing "copy address" that the operation succeeded, and when the QR code modal appears it's not obvious how to close it (other than people's natural reaction to just hit the screen to see what happens)

nop33 · 2022-08-22T17:43:09Z

if you press on an address card and go back, the copy and qr code buttons remain disabled.

Nice catch @LeeAlephium , thanks for the thorough tests!

it's not obvious when pressing "copy address" that the operation succeeded

I'll install a toast message lib because only Android supports it natively 👎 And then @mvaivre can re-iterate on it to improve its styling, since there's nothing in the mockups for notifications atm.

when the QR code modal appears it's not obvious how to close it

The QR code modal is really as basic as I could make it cause I wanna wait for @mvaivre to design one first, so that I don't waste time. I only wanted to make it functional for now. But thanks for the feedback!

LeeAlephium · 2022-08-22T18:44:33Z

src/components/Amount.tsx

+  fullPrecision = false,
+  prefix,
+  suffix = '',
+  hideOnDiscreetMode = true,


Could we rename this to showAlways? It makes logical statements easier to reason about: if (!discreetMode || !hideOnDiscreetMode) { vs if (!discreetMode || showAlways) {. The former is like 3 negatives 🙃 "or if not not showing on not showing amounts mode" .... 😆 Just make sure that the change includes showAlways = false

I'll rename it to showOnDiscreetMode :)

LeeAlephium · 2022-08-22T18:52:05Z

Functional tests look good 👍 My remaining comments are not necessary

nop33 added 5 commits August 18, 2022 13:58

Cleanup in wallet screen headers

96efbbb

Cleanup ScreenSection

6109320

Create basic transaction screen

72d46a7

Add tx link to explorer

61acbb0

Simplify AddressBadge

b6c2e06

nop33 requested a review from LeeAlephium August 18, 2022 11:59

Create addresses carousel

2b8efe7

LeeAlephium suggested changes Aug 18, 2022

View reviewed changes

Add carousel pagination

606cdbf

nop33 changed the title ~~Create transaction screen~~ Create transaction screen and rework addresses screen Aug 18, 2022

nop33 added 2 commits August 18, 2022 17:29

Remove unused prop

efe3b75

Inline

ad36b2d

nop33 added 3 commits August 18, 2022 18:18

Improve conditional

ee3ba43

Remove unnecessary component

aa42a03

Change color

0b196b4

Throw error when button is empty

6093ea1

Do not allow null or undefined in HeaderLeft

875de5d

nop33 requested a review from LeeAlephium August 22, 2022 08:16

nop33 changed the title ~~Create transaction screen and rework addresses screen~~ WIP: Create transaction screen and rework addresses screen Aug 22, 2022

LeeAlephium reviewed Aug 22, 2022

View reviewed changes

nop33 added 5 commits August 22, 2022 19:53

Re-enable address buttons when going back

91d2b59

Show toast message on address copy

bf7d253

Remove unused code

88cebc0

Improve carousel

2b60b6d

Do not hide fee in discreet mode

a178472

nop33 changed the title ~~WIP: Create transaction screen and rework addresses screen~~ Create transaction screen and rework addresses screen Aug 22, 2022

nop33 requested a review from LeeAlephium August 22, 2022 18:24

LeeAlephium reviewed Aug 22, 2022

View reviewed changes

LeeAlephium self-requested a review August 22, 2022 18:52

LeeAlephium approved these changes Aug 22, 2022

View reviewed changes

Rename to showOnDiscreetMode

795e081

nop33 merged commit 503b798 into master Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create transaction screen and rework addresses screen #38

Create transaction screen and rework addresses screen #38

nop33 commented Aug 18, 2022

LeeAlephium commented Aug 18, 2022

LeeAlephium commented Aug 18, 2022 •

edited

Loading

nop33 commented Aug 22, 2022

nop33 commented Aug 22, 2022

LeeAlephium commented Aug 22, 2022

mvaivre commented Aug 22, 2022

nop33 commented Aug 22, 2022

nop33 commented Aug 22, 2022

nop33 commented Aug 22, 2022

nop33 commented Aug 22, 2022 •

edited

Loading

mvaivre commented Aug 22, 2022 •

edited

Loading

LeeAlephium left a comment

nop33 commented Aug 22, 2022

LeeAlephium Aug 22, 2022

nop33 Aug 23, 2022

LeeAlephium commented Aug 22, 2022 •

edited

Loading

Create transaction screen and rework addresses screen #38

Create transaction screen and rework addresses screen #38

Conversation

nop33 commented Aug 18, 2022

LeeAlephium commented Aug 18, 2022

LeeAlephium commented Aug 18, 2022 • edited Loading

nop33 commented Aug 22, 2022

nop33 commented Aug 22, 2022

LeeAlephium commented Aug 22, 2022

mvaivre commented Aug 22, 2022

nop33 commented Aug 22, 2022

nop33 commented Aug 22, 2022

nop33 commented Aug 22, 2022

nop33 commented Aug 22, 2022 • edited Loading

mvaivre commented Aug 22, 2022 • edited Loading

LeeAlephium left a comment

Choose a reason for hiding this comment

nop33 commented Aug 22, 2022

LeeAlephium Aug 22, 2022

Choose a reason for hiding this comment

nop33 Aug 23, 2022

Choose a reason for hiding this comment

LeeAlephium commented Aug 22, 2022 • edited Loading

LeeAlephium commented Aug 18, 2022 •

edited

Loading

nop33 commented Aug 22, 2022 •

edited

Loading

mvaivre commented Aug 22, 2022 •

edited

Loading

LeeAlephium commented Aug 22, 2022 •

edited

Loading