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.

Sign up for GitHub

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

Jump to bottom

event logs tab #1956

Merged
merged 10 commits into from
May 20, 2019
Merged

event logs tab #1956

merged 10 commits into from
May 20, 2019

Conversation

ayrat555
Copy link
Contributor

@ayrat555 ayrat555 commented May 15, 2019 •
edited by vbaranov
Loading

a part of #1927

img

Changelog

  • add logs tab to address

@ghost ghost assigned ayrat555 May 15, 2019
@ghost ghost added the in progress label May 15, 2019
@ayrat555 ayrat555 changed the title event logs tab [WIP] event logs tab May 15, 2019
@coveralls
Copy link

coveralls commented May 15, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build accb8a55-8ae1-4674-8863-ca3a610a9dbb

  • 8 of 19 (42.11%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 81.135%
Changes Missing Coverage Covered Lines Changed/Added Lines %
apps/block_scout_web/lib/block_scout_web/chain.ex 2 3 66.67%
apps/block_scout_web/lib/block_scout_web/views/address_view.ex 0 1 0.0%
apps/block_scout_web/lib/block_scout_web/controllers/address_logs_controller.ex 0 9 0.0%
Totals Coverage Status
Change from base Build 3fb4fd5a-ed70-45c8-964b-2392b1e27ad9: -0.1%
Covered Lines: 4675
Relevant Lines: 5762
💛 - Coveralls

@ayrat555 ayrat555 changed the title [WIP] event logs tab event logs tab May 16, 2019
@ghost ghost assigned vbaranov May 16, 2019
zachdaniel
zachdaniel reviewed May 16, 2019
View reviewed changes
apps/block_scout_web/lib/block_scout_web/templates/address_logs/index.html.eex
<%= render BlockScoutWeb.CommonComponentsView, "_pagination_container.html", position: "top", cur_page_number: "1", show_pagination_limit: true, next_page_path: @next_page_url %>
<% end %>

<%= if !@next_page_url do %>
Copy link
Contributor

Choose a reason for hiding this comment

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

This could just be an else

pasqu4le
pasqu4le approved these changes May 17, 2019
View reviewed changes
Copy link
Contributor

@pasqu4le pasqu4le left a comment

Choose a reason for hiding this comment

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

Just a minor point, but LGTM

apps/explorer/lib/explorer/chain.ex

query
|> Repo.all()
|> Enum.take(paging_options.page_size)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this unnecessary, given that the query already sets this limit?

Also 2 nitpicks:

  • query is only used here, it could be directly piped into Repo.all without an assignment
  • I may be wrong, but it seems several Explorer.Chain.* modules define a specific function to generate this kind of queries, e.g. like CoinBalance

@igorbarinov
Copy link
Member

Is search function a part of the PR or will it be implemented separately?

@ayrat555
Copy link
Contributor Author

@igorbarinov what do you mean by search functions? should we able to search by the transaction?

@igorbarinov
Copy link
Member

Should a be a search box so developers can search for events within smart contract logs across all transactions.
It's formulated in the issue here

"[..] and a search function. Users should be able to search for event logs by topic across the full blockchain."

In the provided example there are > 204,404 transfers in the smart contract and it will be convinient to have a search function for it.

@vbaranov
Copy link
Member

I will merge this. @ayrat555 please add search functionality in a separate PR.

@vbaranov vbaranov merged commit 7abb6ec into master May 20, 2019
@vbaranov vbaranov deleted the ab-logs-tab branch May 20, 2019 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vbaranov vbaranov vbaranov approved these changes

@zachdaniel zachdaniel zachdaniel approved these changes

@saneery saneery saneery approved these changes

@pasqu4le pasqu4le pasqu4le approved these changes

Assignees

@vbaranov vbaranov

@ayrat555 ayrat555

Labels
in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ayrat555 @coveralls @igorbarinov @vbaranov @zachdaniel @saneery @pasqu4le