Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ABW-1547] Ledger signing++ #549

Merged
merged 30 commits into from
Jun 5, 2023
Merged

[ABW-1547] Ledger signing++ #549

merged 30 commits into from
Jun 5, 2023

Conversation

kugel3
Copy link
Contributor

@kugel3 kugel3 commented Jun 2, 2023

Jira ticket: ABW-1547

Description

Fixes Request signing with Ledger, and a number of other issues around the app, mostly in Asset Transfer and Transaction Review.

Note that cancellation of requests is not yet implemented.

What to check:

These are the main things that have changed in the UI:

  • All strings should be up to date

Create New Account

  • The Ledger toggle was green when creating a first account

  • The keyboard had autocorrect

Link New Connector

  • Link New Connector cancel button didn't work

  • Name New Connector had no title

  • If you closed the Name New Connector sheet, a connector was still added, called Unnamed. This should instead cancel the adding.

Name Your Ledger

  • Centered textfield

  • Action button was too small and had too much bottom space

Create account with ledger

  • (Derive Public Keys) was not done

Sign transaction screen

  • was not done

Add New Gateway

  • Close button scrolled

  • Action button was not tied to keyboard

  • When in Ledger Hardware devices and not connected to a Connector you will be asked to connect to one, but the dismiss button of the Link New Connector screen was not working

Fungible Assets

  • Strange appearance

  • Bottom of list was outside screen (also for NFTs)

Non-fungible assets

  • In the list, individual NFTs without a keyImage had a gray bar, this is fixed now, they have nothing

  • Expansion animation was not using easeInOut

  • Check boxes were always visible

Asset transfer

  • Close button was not in a navigation bar

  • In Choose assets: navigation bar and close button

  • If you cancelled a signing request, you ended up on Transaction Review with a disabled Action button

Video

New creating Ledger account flow:

RPReplay_Final1685960061.MP4

PR submission checklist

  • I have tested account to account transfer flow and have confirmed that it works

Comment on lines +38 to +46
.toolbar {
ToolbarItemGroup(placement: .keyboard) {
Spacer()

Button(L10n.Common.done) {
focused = false
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should display a "Done" button right above the keyboard, but it does not seem to work right now, many are experiencing this in iOS 16. Seems like a SwiftUI bug.

Copy link
Contributor

@CyonAlexRDX CyonAlexRDX Jun 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buttons in toolbar (even keyboard) are only visible if the view is wrapped in a NavigationView

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will have a look at that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw I believe we should not use ANY "Common" strings at all. But so "common.done" ought to be split like you did "common.continue". But that can be a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry, I did try wrapping in a navigation view, at various levels, and it still didn't work, so nobody will see this for a while...


default:
return .none
}
}

private func cancelSigningEffect(state: inout State) -> EffectTask<Action> {
// FIXME: How to cancel?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nil state.destination... I think should be enought.

@kugel3 kugel3 changed the title Ledger signing++ [ABW-1547] Ledger signing++ Jun 5, 2023
Copy link
Contributor

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! LGTM!

@kugel3 kugel3 merged commit e6b5ac2 into main Jun 5, 2023
@kugel3 kugel3 deleted the ledger-signing branch June 5, 2023 20:17
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants