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

Account: Saved Keypairs #1138

Merged
merged 6 commits into from
Nov 7, 2024
Merged

Account: Saved Keypairs #1138

merged 6 commits into from
Nov 7, 2024

Conversation

quietbits
Copy link
Contributor

@quietbits quietbits commented Nov 6, 2024

image

image

image

@quietbits quietbits linked an issue Nov 6, 2024 that may be closed by this pull request
@stellar-jenkins
Copy link

@quietbits quietbits requested a review from jeesunikim November 6, 2024 18:27
@janewang
Copy link
Contributor

janewang commented Nov 6, 2024

@quietbits On the Save Keypair page, should we follow the same pattern as on the Create Account Keypair page and hide the secret key as ********. What do you think?

Separately, when my mouse hovers over the public key, my cursor turns to this 🚫 . I think it should be the same as the Create Account Keypair page.

@quietbits
Copy link
Contributor Author

@quietbits On the Save Keypair page, should we follow the same pattern as on the Create Account Keypair page and hide the secret key as ********. What do you think?

@janewang , yes, we can mask the secret key. 👍

Separately, when my mouse hovers over the public key, my cursor turns to this 🚫 . I think it should be the same as the Create Account Keypair page.

The input fields are disabled, that's why you see the 🚫 icon on hover. We can change that to read-only, but the design has the disabled style. It's an easy update.

image

@janewang
Copy link
Contributor

janewang commented Nov 6, 2024

The input fields are disabled, that's why you see the 🚫 icon on hover. We can change that to read-only, but the design has the disabled style. It's an easy update.

I think the design should be consistent in saved keypairs and in create keypairs. Thank you for the update!

@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

@jeesunikim
Copy link
Contributor

I think we should reset the generate keypair inputs when switching from mainnet to futurenet. its public key input stays intact without private key input.

mainnet-to-futurenet.mov

@jeesunikim
Copy link
Contributor

Fund Account and Saved Keypairs styling on mainnet should match

saved-keypairs-styling friendbot-styling

import { NetworkType, SavedKeypair } from "@/types/types";
import { arrayItem } from "@/helpers/arrayItem";

export default function SavedKeypairs() {
Copy link
Contributor

Choose a reason for hiding this comment

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

curious - when should we use export default function vs. export const. in vibrant days, we preferred using const for exporting vs. default (product convention).

Copy link
Contributor

Choose a reason for hiding this comment

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

(I am not suggesting that we follow the convention btw, I'm just curious)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case (page.tsx), we need to follow the Next.js convention and any other framework level file (examples). I agree that we should use named exports when possible. 👍

getNetworkById(newNetworkId);
}}
>
{`You must switch your network to ${otherNetworkLabel} in order to see those saved
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just do the following?
You must switch your network to {otherNetworkLabel} in order to see those saved keypairs. This feature is only available on Futurenet and Testnet for security reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that works, too. In cases like this, I prefer formatting it as a string because it's more clear and consistent in the code editor what it is. This is just my preference, nothing else. 😊 If you feel strongly about it, I can update it.

image

vs

image

<Text
as="div"
size="xs"
>{`Last saved ${formatTimestamp(timestamp)}`}</Text>
Copy link
Contributor

Choose a reason for hiding this comment

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

can we do Last saved {formatTimestamp(timestamp)} instsead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

@quietbits
Copy link
Contributor Author

I think we should reset the generate keypair inputs when switching from mainnet to futurenet. its public key input stays intact without private key input.

mainnet-to-futurenet.mov

Good idea, we should reset the keypair when the network changes. Issue created.

@quietbits
Copy link
Contributor Author

Fund Account and Saved Keypairs styling on mainnet should match

saved-keypairs-styling friendbot-styling

Good catch! 🙌 We have two types of headings throughout the app. I started a Slack thread to see how we want to handle them.

@stellar-jenkins
Copy link

@quietbits quietbits merged commit a775d84 into master Nov 7, 2024
3 checks passed
@quietbits quietbits deleted the saved-keypairs branch November 7, 2024 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Account] Saved Keypairs
4 participants