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

fix: register ledger device #1746

Merged

Conversation

mateuszjasiuk
Copy link
Collaborator

@mateuszjasiuk mateuszjasiuk commented Feb 25, 2025

There is one annoying thing that after going back from settings we have to choose asset once again, but we can improve it later.

register.device.mp4

@mateuszjasiuk mateuszjasiuk marked this pull request as ready for review February 26, 2025 07:43
@mateuszjasiuk mateuszjasiuk requested review from jurevans, pedrorezende and euharrison and removed request for jurevans and pedrorezende February 26, 2025 07:43
@@ -4,14 +4,16 @@ import { twMerge } from "tailwind-merge";

type IconTooltipProps = {
icon: React.ReactNode;
text: string;
text: string | React.ReactNode;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could just be React.ReactNode - I believe that type includes string, number, etc

</p>
<p className="text-sm">
The <b>Register Ledger Device</b> button remains active because we have
no way to check if a device is already registered.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would maybe reword this:

The Register Ledger Device button remains active because we have
no way to check if a device is already registered.

It is normal to see the Register Ledger Device button after registering due to limitations of Ledger registration detection in browser.

...

Or something. Hmmm, I dunno, it just feels a little weird, but is likely fine! I get why it should be here

Copy link
Collaborator

@jurevans jurevans left a comment

Choose a reason for hiding this comment

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

This looks really good! Take or leave my comments, they are very minor suggestions!

@mateuszjasiuk mateuszjasiuk force-pushed the feat/ledger-masp-integration-branch branch from d2717e3 to 5174dd5 Compare February 26, 2025 11:15
@mateuszjasiuk mateuszjasiuk force-pushed the fix/register-ledger-device branch from 7270cb0 to a87555f Compare February 26, 2025 11:29
@mateuszjasiuk mateuszjasiuk merged commit d5ac94a into feat/ledger-masp-integration-branch Feb 27, 2025
7 checks passed
@mateuszjasiuk mateuszjasiuk deleted the fix/register-ledger-device branch February 27, 2025 08:59
jurevans pushed a commit that referenced this pull request Feb 27, 2025
* fix: register ledger device

* feat: refactor + cleanup

* fix: code review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants