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-3581] Inform users when signing with Ledger without P2PLinks #1343

Merged
merged 3 commits into from
Oct 16, 2024

Conversation

matiasbzurovski
Copy link
Contributor

@matiasbzurovski matiasbzurovski commented Sep 16, 2024

Jira ticket: ABW-3581

Description

Inform users when they attempt to access a Ledger factor source but the wallet isn't connected to any connector extension.
Also, improves the UX by removing a glitch where the ledger name wouldn't show up for an instant.

Videos

Before After
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-09-16.at.16.48.58.mp4
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-09-16.at.17.32.04.mp4

Copy link
Contributor

@danvleju-rdx danvleju-rdx left a comment

Choose a reason for hiding this comment

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

LGTM

return .none
}
return .run { send in
for try await isConnected in await ledgerHardwareWalletClient.isConnectedToAnyConnectorExtension() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is what we should check. This can return false if there are P2PLinks, but these are not connected(you browser is closed), I believe that the intention was to inform users when there are no P2PLinks at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, will update it to check if there are any p2pLinks instead.

Do we want to leave it this way when adding a new Ledger device? Because in such case we will show this alert if there is no active connection, and user could be lead to add a new one when all they need to do is open their browser

@GhenadieVP GhenadieVP added the DO NOT MERGE Merging is blocked or prohibited due to missing specs or other higher priority PRs label Sep 24, 2024
@GhenadieVP GhenadieVP removed the DO NOT MERGE Merging is blocked or prohibited due to missing specs or other higher priority PRs label Oct 16, 2024
@matiasbzurovski matiasbzurovski merged commit cda5737 into main Oct 16, 2024
6 checks passed
@matiasbzurovski matiasbzurovski deleted the ABW-3581-ledger-without-connector branch October 16, 2024 15:29
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.

4 participants