Skip to content
This repository was archived by the owner on Apr 4, 2022. It is now read-only.

Search by OrderId and Address account #752

Merged
merged 6 commits into from
Oct 14, 2021
Merged

Conversation

henrypalacios
Copy link
Contributor

@henrypalacios henrypalacios commented Oct 13, 2021

Summary

Closes #691

Proposal:

  • Allow search by OrderId and Address Account
  • Otherwise, show the not found page

To Test

  1. Go to explorer search
    2.1 Search by Address account -> i.e: 0x04a66cbba0485d7b21af836f52b711401300fddb
    2.2 Search by Order Id -> i.e: 0x88537c74a03c324464d6c269d99b29e7289b105766c5368f77c76c241e1d5da8b6bad41ae76a11d10f7b0e664c5007b908bc77c9615b2dac

@henrypalacios henrypalacios self-assigned this Oct 13, 2021
@henrypalacios henrypalacios added app:Explorer Explorer App Protofire task to the protofire team labels Oct 13, 2021
@github-actions
Copy link

@elena-zh
Copy link

Changes LGTM: I'm able to search by an order ID and an address (full match).
But an address info is available in the Mainnet only, even if I search for it when the explorer is in XDAI network
https://watch.screencastify.com/v/UvUk0c1NJENLRponEaYG

I hope, it will be fixed in #747

@henrypalacios
Copy link
Contributor Author

Thanks @elena-zh for the feedback, Then I think it is best to keep the URL network prefix in the search.
Let me try!

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Looking good, loved the tests

Good idea updating the text placeholder. We should also update the header
screenshot_2021-10-13_07-11-04

And, I think we should change the behaviour a little bit when nothing is found.

With this PR, it goes to /search/whatever and we show the 404 page
screenshot_2021-10-13_07-16-40

But I think the best would be to update and use the currect /order/whatever path on /search/whatever
screenshot_2021-10-13_07-12-48

Same thing that you did here, update the texts/placeholders and use that on search

What do you think of that?

@alfetopito
Copy link
Contributor

Changes LGTM: I'm able to search by an order ID and an address (full match). But an address info is available in the Mainnet only, even if I search for it when the explorer is in XDAI network https://watch.screencastify.com/v/UvUk0c1NJENLRponEaYG

I hope, it will be fixed in #747

It might be fixed by that, only if you are already in the network you want.

The "ultimate" fix would be to implement the address search like we have for order ids #711

In any case, at this point I'm ok not having that in favor of releasing it

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Almost perfect

Only thing I'd like is to have the "not found" search results page to be under /search instead of /orders

screenshot_2021-10-13_10-44-39

Maybe this is a different task/PR?

I'm ok merging as is to keep the PR concise, if that change ends up being too big.

I'll leave it up to you to address that here or not

Copy link
Contributor

@alongoni alongoni left a comment

Choose a reason for hiding this comment

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

Looks and

Copy link
Contributor

@alongoni alongoni left a comment

Choose a reason for hiding this comment

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

Looks and works fine!

(Only one comment regarding search box on order not found page)

@henrypalacios
Copy link
Contributor Author

@alfetopito I have tried to go for the not found change, but it is convenient to create another PR because it may alter other things.

@henrypalacios henrypalacios changed the base branch from develop to release/2.3.0 October 13, 2021 19:31
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Awesome

@anxolin
Copy link
Contributor

anxolin commented Oct 14, 2021

Not for this release, we really need it out, but maybe we can hint somehow the network the user is looking for orders.

For example, I used an account that only traded in Rinkeby, I looked for 0xe3a60C046Bcf663645d55EA7eD6E1eD167bd1dF0 and it shows:

image

One small thing we can do is to change the message to make sure the user is aware of the network he/she is in. Ideally with a way to switch networks easily. Probably this could be connected to the network switcher task.

Additionally, and as a good to have, the search algorithm could do exactly what the find by id is doing:

  • Look in the current network
  • If no results, try to find in another network
  • if there's results, forward the user there

EXAMPLE:

@elena-zh
Copy link

Changes LGTM.
A tiny issue: to start 'order' word with the capital letter in the placeholder
order with capital

@henrypalacios
Copy link
Contributor Author

@anxolin I think the approach is great!

Not for this release, we really need it out, but maybe we can hint somehow the network the user is looking for orders.
...
Look in the current network
If no results, try to find in another network
if there's results, forward the user there

@alfetopito can I create a PR for this?

@elena-zh it's done!

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Thanks!

@alfetopito
Copy link
Contributor

@anxolin I think the approach is great!

Not for this release, we really need it out, but maybe we can hint somehow the network the user is looking for orders.
...
Look in the current network
If no results, try to find in another network
if there's results, forward the user there

@alfetopito can I create a PR for this?

Yes, please!

This is actually one of the follow ups I mentioned. Very good to have but not a blocker for the release

@alfetopito alfetopito added the Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds label Oct 14, 2021
@mergify mergify bot merged commit 2b2193a into release/2.3.0 Oct 14, 2021
@alfetopito alfetopito deleted the 691-search-by-address branch October 14, 2021 17:50
@alfetopito alfetopito mentioned this pull request Oct 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
app:Explorer Explorer App Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds Protofire task to the protofire team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search by address
5 participants