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

Add transaction status indicator #412

Merged
merged 19 commits into from
Jul 17, 2018
Merged

Conversation

arthurra
Copy link
Contributor

@arthurra arthurra commented Jul 16, 2018

resolves #368
resolves #400

Motivation

Visually show the current status of a given transaction. Current statuses are Success, Pending, Failed and Out of Gas. Failed and Out of Gas transactions are given a colored border to call attention to them. Transaction statuses will also live update as blocks are validated.

Changelog

Enhancements

  • Added a text label that describes the transaction's status
  • Added a colored border to call attention to Failed and Out of Gase transactions

Screenshots

localhost_4000_en_addresses_0x00f5ba0190675051175eacbc21b2dc649887d3b9_transactions

@arthurra arthurra added the wip label Jul 16, 2018
@ghost ghost assigned arthurra Jul 16, 2018
@ghost ghost added the in progress label Jul 16, 2018
@arthurra arthurra changed the title Add transaction status indicator #400 Add transaction status indicator Jul 16, 2018
@arthurra arthurra added ready for review This PR is ready for reviews. and removed wip labels Jul 16, 2018
@acravenho
Copy link
Contributor

screen shot 2018-07-16 at 7 23 00 pm

The all transactions page isn't highlighting the failed transaction state like it is on the address page.

@acravenho
Copy link
Contributor

acravenho commented Jul 16, 2018

Before:
screen shot 2018-07-16 at 7 50 34 pm

After:
screen shot 2018-07-16 at 7 50 22 pm

@SiddigZeidan a little spacing there looks nice
WDYT?

) %>
<div class="col-md-3 col-lg-2 d-flex flex-row flex-md-column justify-content-start text-md-right">
<span class="tile-title">
<%= ExplorerWeb.TransactionView.value(transaction, include_label: false) %> POA
Copy link
Contributor

Choose a reason for hiding this comment

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

POA should be localized.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! Updated this in the last push!

to: block_path(@conn, :show, @conn.assigns.locale, transaction.block)
) %>
<div class="col-md-3 col-lg-2 d-flex flex-row flex-md-column justify-content-start text-md-right">
<span class="tile-title"><%= ExplorerWeb.TransactionView.value(transaction, include_label: false) %> POA</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

POA should be localized.

Copy link
Contributor

Choose a reason for hiding this comment

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

This one too!

<%= if @address.hash == @transaction.from_address_hash do %>
<span class="badge badge-danger tile-badge">Out</span>
<% else %>
<span class="badge badge-success tile-badge">In</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Out and In need gettext

Copy link
Contributor

Choose a reason for hiding this comment

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

In is IN in address_internal_transaction/_internal_transaction.html.eex, the capitalization should be consistent across templates.

) %>
<div class="col-md-3 col-lg-2 d-flex flex-row flex-md-column justify-content-start align-items-end text-md-right">
<span class="tile-title">
<%= ExplorerWeb.TransactionView.value(@transaction, include_label: false) %> POA
Copy link
Contributor

Choose a reason for hiding this comment

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

POA needs gettext

@@ -13,5 +13,6 @@ defmodule ExplorerWeb.AddressTransactionView do
end
end

defdelegate formatted_status(transaction), to: TransactionView
Copy link
Contributor

@jimmay5469 jimmay5469 Jul 17, 2018

Choose a reason for hiding this comment

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

In some places we've been using defdelegate and in others we've been using fully qualified function names in the template ExplorerWeb.TransactionView.formatted_status(. I personally prefer the fully qualified names in the template because it seems more explicit but am fine with defdelegate if that is what others prefer. @KronicDeth @alexgaribay @igorffs thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jimmay5469, I think you meant @igorffs

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the full call or to call the function directly in the template.

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer full call. Less magic, more explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress ready for review This PR is ready for reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add transaction status indicator Update transactions list page design
7 participants