Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

simplify tx confirmations display #3559

Merged
merged 9 commits into from
Nov 28, 2016
Merged

simplify tx confirmations display #3559

merged 9 commits into from
Nov 28, 2016

Conversation

derhuerst
Copy link
Contributor

@derhuerst derhuerst commented Nov 21, 2016

This PR makes TxHash look a lot less verbose, aligning the UI more with targeted user group. Advanced users still get the block number when hovering.

I added a new ui/Hash ui/ShortenedHash component, which is supposed to be put in every place where a tx hash is shown. Will also expand this to address hashes.

Except the obvious changes, there is a small visual regression in this PR: The tx hash in the Signer is now so short that the left-alignment of the text is visible.

signer-regression

@derhuerst derhuerst added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Nov 21, 2016
@derhuerst
Copy link
Contributor Author

Also thought about using a non-linear scale to visualise the change of a tx getting rejected by the network properly.

const { data } = this.props;

let shortened = data.toLowerCase();
if (shortened[0] === '0' && shortened[1] === 'x') {
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason for not just checking shortened.substr(0, 2) === '0x' (as done everywhere else - consistency so we don't need to think about how xyz does it)

if (confirmations.gt(maxConfirmations)) {
count = confirmations.toFormat(0);
} else {
count = confirmations.toFormat(0) + '/' + maxConfirmations;
Copy link
Contributor

Choose a reason for hiding this comment

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

ES6 string template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You will always be able to argue with the point of consistency and I agree that it is important. Readable code is more important though imo.

I find

foo + '/' + bar

a lot less noisy than

`${a}/${b}`

Will adapt to whatever you decide, but keep this in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be balances between the two of you. IMO:

count = `${confirmations}/${maxConfirmations}`

and

count = confirmations.toFormat(0) + `/${maxConfirmations}`

Ie. if just variables, then string template, if returned value of method, + concat.

@jacogr
Copy link
Contributor

jacogr commented Nov 21, 2016

Good, right step. Like you suggested, would probably just fix up the Signer display a little bit so it looks presentable. (Alternatively just log an issue for it, I have all the Signer items in my queue, so busy reworking the full display & adding the required additional functionality)

@derhuerst
Copy link
Contributor Author

Good, right step. Like you suggested, would probably just fix up the Signer display a little bit so it looks presentable. (Alternatively just log an issue for it, I have all the Signer items in my queue, so busy reworking the full display & adding the required additional functionality)

I'd propose to have a different way of showing that a TX is confirmed/rejected/pending. Just putting it in a column the importance of the information. I suggest we decide on a new look for the Signer as a group.

Until that happened, I'll center the "Rejected"/"12 Confirmations" messages.

@derhuerst derhuerst added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 22, 2016
@derhuerst derhuerst added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Nov 24, 2016
@jacogr jacogr added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 24, 2016
@jacogr
Copy link
Contributor

jacogr commented Nov 24, 2016

Something looks weird here - it contains files from other PRs, not sure what was done here and what not.

@derhuerst
Copy link
Contributor Author

Something looks weird here - it contains files from other PRs, not sure what was done here and what not.

The commits that are actually part of this PR (and not merged in) are the following:

  1. 97db6e6
  2. 0d832cb
  3. 35c265f
  4. afc0687
  5. 5bc6ed5
  6. 85227f9
  7. 14984c3

@derhuerst derhuerst added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Nov 28, 2016
@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 28, 2016
@jacogr jacogr merged commit 2b178d8 into master Nov 28, 2016
@jacogr jacogr deleted the jr-better-txhash branch November 28, 2016 16:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants