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: advanced search #804

Merged
merged 54 commits into from Jan 13, 2020
Merged

Conversation

@Lemii
Copy link
Contributor

Lemii commented Jan 2, 2020

Summary

This PR adds an advanced search page to the explorer, as described in the following Tier-0 Bounty: [explorer][750 USD] Advanced Search Page

Apart from the implementation of the new functionality, there was also some minor refactoring done. For example, the IApiDelegateVotersWrapper has been refactored to IApiWalletsWrapper, as it can accomodate both objects. Also, the wallet search service has been adjusted to make more similar to the other already existing services.

Checklist

  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged
@Lemii

This comment has been minimized.

Copy link
Contributor Author

Lemii commented Jan 7, 2020

some other things I ran into when I tried to use the search:

  • transactions/blocks/wallets; console gives a warning about wallets being "", this probably means they are rendered before they are actually loaded properly

Still need to look into this one. It appears that the router falls back to a 2.0 redirect for some reason and therefore expects an address parameter..

    {
      path: "/address/:address",
      redirect: to => ({
        name: "wallet",
        params: { address: to.params.address },
      }),
      meta: { title: (route: Route) => getTitle("Wallet") },
    },
  • transaction: when I search for 31c17d3a76692a111156e59d605caff78bd359eaca50b7abe24fa4be86e7e26f it finds one transaction (expected), but if I changed amount from to 1, it suddenly no longer finds anything

This ID is of a multipayment transaction. The actual amount of that transaction itself is 0. The API does not query the sum of the multipayments that are inside of it.

  • can you set a default value in the transaction type dropdown (just transfer by default)
  • wallets; when you search for something that has no results, the table headings jump around for a second
  • wallets; when you use a delegate that does not exist with voting for (e.g. itsanametoo1234), it returns all wallets instead of none. Same happens in the block search when filling in a non-existent delegate in the generated by field.
  • wallets; when you start typing in the balance from field and enter a 0, it throws an error in the console (e.g. if you want to search for 0.001 as balance). Also happens with other number fields like total fee.
  • the sort settings for the wallet results are shared with the voters table and top-wallets tables and can result in errors in the console (e.g. set top-wallets to sort by rank) and then search for wallets and it will throw an error about sortFn being undefined as that column does not exist there.

Other than that, it works great overall. Only some minor issues that need to be looked at

These issues have now been resolved.

@ItsANameToo

This comment has been minimized.

Copy link
Member

ItsANameToo commented Jan 8, 2020

I looked into the router issue. It's not trying to use the fallback, but this one:

{
      path: "/wallets/:address",
      name: "wallet",
      component: WalletComponent,
      meta: { title: (route: Route) => getTitle("Wallet " + route.params.address) },
    },

which is used by components such as LinkWallet. The issue you're having is that the AdvancedSearch component lists all options (transactions, blocks, wallets), but is missing a v-if for the mobile wallet search. So on every search you do, it will try to create that in the DOM (but hidden)and since you might be searching for transactions, it can't link to a proper wallet and ends up giving "" to the component. I've added a comment that should fix the issue

Only thing remaining then is that if you search for a wallet, the voting for can be empty and it will still pass "" to the router, so it will need a check there if the wallet is actually voting so it doesn't unnecessarily links to routes

@ItsANameToo

This comment has been minimized.

Copy link
Member

ItsANameToo commented Jan 8, 2020

Regarding the multipayment amount issue, good catch I hadn't linked the two :trollface: Will tell the core guys to look into that

Lemii added 5 commits Jan 8, 2020
src/pages/AdvancedSearch.vue Outdated Show resolved Hide resolved
src/store/modules/ui.ts Outdated Show resolved Hide resolved
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

ArkEcosystemBot commented Jan 13, 2020

A member has approved this PR. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait.

Thank you for your contribution!

@ItsANameToo ItsANameToo merged commit 677e9e6 into ArkEcosystem:develop Jan 13, 2020
6 of 7 checks passed
6 of 7 checks passed
e2e (12.x)
Details
unit (12.x)
Details
build-devnet (12.x)
Details
build-mainnet (12.x)
Details
build-testnet (12.x)
Details
codeclimate 6 issues to fix
Details
security/snyk - package.json (ArkEcosystem) No manifest changes detected
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

ArkEcosystemBot commented Jan 13, 2020

Your pull request has been merged but was not assigned a bounty tier. A maintainer will assign a bounty tier to this pull request in the next few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.