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

feat: add nonce indicator on pending transactions when viewing an address (#622) #624

Closed
wants to merge 6 commits into from

Conversation

linden
Copy link

@linden linden commented Jan 22, 2022

Adds a indicator next to pending transactions, reasons detailed here (#622).

@linden
Copy link
Author

linden commented Jan 22, 2022

I've got 3 options for how this could be displayed:

A:
image
Simple, although looks awkward when compared to non-pending transactions.

B:
image
Easy to associate with a pending transaction.

C:
image
Blends in, would need to add some sort of hover-over description.

Any feedback on what looks the best?

@kyranjamie
Copy link
Contributor

Great stuff, thanks for the contribution. Def C with a tooltip. Maybe the number styleized as e.g. 17n? 🤔

@314159265359879
Copy link

314159265359879 commented Jan 22, 2022

Thanks for picking this up @linden here are my thoughts:

A is most descriptive, but I like C most because it is least noticeable and I think that is fine in most cases. I think it is clever to put in a line we were already using as to not increase the height required per item shown.

What about C including the word nonce?
Pending - 0xa31d...f886 - nonce: 14

@314159265359879
Copy link

What Kyran says even better 14n, the hover will do the rest.

@fbwoolf
Copy link
Contributor

fbwoolf commented Jan 22, 2022

Great stuff, thanks for the contribution. Def C with a tooltip. Maybe the number styleized as e.g. 17n? 🤔

I like this idea bc on the home page there is limited space across. I don't think spelling out nonce is necessary? I would say C is best and add the n

@linden
Copy link
Author

linden commented Jan 22, 2022

Thanks for the feedback! Here's what it looks like with the tooltip and the n.

explorer.nonce.mov

@kyranjamie kyranjamie marked this pull request as ready for review January 22, 2022 18:44
Copy link

@314159265359879 314159265359879 left a comment

Choose a reason for hiding this comment

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

LGTM

@markmhendrickson markmhendrickson added Enhancement 💡 Functionality needed or wanted by users P3 Enhancement ✨ Non-critical functionality wanted by many users, or there are clear alternatives labels Apr 4, 2022
@Hero-Gamer
Copy link

Great stuff, thanks for the contribution. Def C with a tooltip. Maybe the number styleized as e.g. 17n? 🤔

I like this idea bc on the home page there is limited space across. I don't think spelling out nonce is necessary? I would say C is best and add the n

I'll speak from users perspective if I may?

as much as I like "n"
I think spelled out "Nonce" is easier for the less advanced users, which there are a lot across the ecosystem. One less thing for us having to explain. There should be plenty of space on the page (majority uses desktop to access Explorer)

I'd also like to suggest add "Fees" to the Explorer. Both Explorer and Hiro wallet if possible.
Sorry!

I've made the comment on separate issues:
#622
leather-io/extension#2129

Copy link

@landitus landitus left a comment

Choose a reason for hiding this comment

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

Even though I see good points for making the nonce more discoverable for new users, I'm thinking it's best to keep the line items as simple as possible given they repeat multiple times. Right now, each information doesn't have a label, which is a good decision to avoid repeating text and crowding the interface.

Having the tooltip as shown here should do the trick now.

For adding more information, such as fees, it will be more challenging (since there are no labels to explain what each piece of information means). I think then it may be time to review the line item's design a bit further.

@andresgalante
Copy link
Member

@He1DAr Fernando is ok with the design, let's move on to a code review please

Copy link
Collaborator

@He1DAr He1DAr left a comment

Choose a reason for hiding this comment

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

LGTM! left a couple of small comments. Could you resolve CI issues?

src/components/transaction-item.tsx Outdated Show resolved Hide resolved
src/components/transaction-item.tsx Outdated Show resolved Hide resolved
@linden
Copy link
Author

linden commented May 17, 2022

Looks ready to me, anything else needed @He1DAr?

@blockstack-devops
Copy link

🎉 This issue has been resolved in version 1.20.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@314159265359879
Copy link

This is brilliant, I love it!
Makes it so much easier to troubleshoot nonce issues.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement 💡 Functionality needed or wanted by users P3 Enhancement ✨ Non-critical functionality wanted by many users, or there are clear alternatives released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show nonces on (pending) transactions viewing an address in the explorer
10 participants