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

Search across all networks #408

Merged
merged 1 commit into from May 24, 2023
Merged

Search across all networks #408

merged 1 commit into from May 24, 2023

Conversation

csillag
Copy link
Contributor

@csillag csillag commented May 22, 2023 •
edited

Task is https://app.clickup.com/t/862ju7v0a.

This depends on #404.

Some explanation: with this, we support two kinds of search.

  • One is launched from within a page belonging to a specific network. The URL is like /mainnet/ember/search?q=whatever. In this case, we are mainly showing the results from that network. (And maybe some indicators about results elsewhere.)
  • Then there is the so-called "global" search, which is what you get if you launch a search right from the opening homepage, without actually entering any paratime first. The URL for this is /global/search?q=whatever. In this case, you see all results, grouped by networks of course.

In the URL, there is always something that specifies the flavor of the search. That is either a name of a network (mainnet or testnet), or "global" for global search.

@csillag csillag force-pushed the csillag/network-cross-search branch from e036e58 to 1d43006 Compare May 22, 2023 00:13
@github-actions
Copy link

github-actions bot commented May 22, 2023 •
edited

Deployed to Cloudflare Pages

Latest commit: 7a2d0fa31e39859cc2aab16093ff046b893a50d9
Status:✅ Deploy successful!
Preview URL: https://f25366fd.oasis-explorer.pages.dev

@csillag csillag force-pushed the csillag/network-cross-search branch 2 times, most recently from aa7ebcb to bdd25ff Compare May 22, 2023 11:02
@csillag csillag mentioned this pull request May 22, 2023
Merged
@csillag csillag marked this pull request as ready for review May 22, 2023 12:03
@csillag csillag force-pushed the csillag/network-cross-search branch from 8d51111 to b5fc285 Compare May 22, 2023 16:28
@csillag csillag mentioned this pull request May 22, 2023
Merged
@csillag csillag force-pushed the csillag/network-cross-search branch from b5fc285 to 91d4fc6 Compare May 22, 2023 16:34
@csillag csillag mentioned this pull request May 22, 2023
Merged
@csillag csillag force-pushed the csillag/network-cross-search branch from 91d4fc6 to 2041c09 Compare May 22, 2023 21:47
@csillag csillag mentioned this pull request May 22, 2023
Closed
lukaw3d
lukaw3d reviewed May 22, 2023
View reviewed changes
src/styles/index.css Outdated Show resolved Hide resolved
@csillag csillag force-pushed the csillag/network-cross-search branch from 2041c09 to d0482aa Compare May 23, 2023 05:48
@csillag csillag requested a review from buberdds May 23, 2023 05:49
@csillag
Copy link
Contributor Author

csillag commented May 23, 2023

Rebased this again, all dependencies has been merged, now this is ready for review.

Please note that there are two known issues which this PR doesn't handle:

@csillag csillag force-pushed the csillag/network-cross-search branch from d0482aa to 8daeb2c Compare May 23, 2023 14:37
lukaw3d
lukaw3d approved these changes May 23, 2023
View reviewed changes
src/app/components/NetworkThemeBubble/index.tsx Outdated Show resolved Hide resolved
src/routes.tsx Outdated Show resolved Hide resolved
src/locales/en/translation.json Outdated Show resolved Hide resolved
src/app/pages/SearchResultsPage/ResultsOnForeignNetwork.tsx Outdated Show resolved Hide resolved
src/app/pages/SearchResultsPage/ResultsOnForeignNetwork.tsx Outdated
if (!open) {
return (
<NetworkThemeBubble network={network}>
<NotificationBox className={'foreignNote'} onClick={() => setOpen(true)}>
Copy link
Member

Choose a reason for hiding this comment

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

<Box onClick is horrible for accessibility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, which component do you suggest we use here, insted of Box?

Copy link
Member

Choose a reason for hiding this comment

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

Probably Collapse

Copy link
Member

Choose a reason for hiding this comment

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

And Button

src/app/pages/SearchResultsPage/ResultsOnNetwork.tsx Show resolved Hide resolved
src/app/pages/SearchResultsPage/index.tsx Outdated Show resolved Hide resolved
src/app/pages/SearchResultsPage/ResultsOnNetwork.tsx Outdated Show resolved Hide resolved
src/app/pages/SearchResultsPage/SearchResultsList.tsx Outdated Show resolved Hide resolved
src/app/pages/SearchResultsPage/ResultsOnForeignNetwork.tsx Outdated Show resolved Hide resolved
lukaw3d
lukaw3d reviewed May 23, 2023
View reviewed changes
src/app/pages/SearchResultsPage/SearchResultsList.tsx Outdated
<>
<ResultsGroup
title={t('search.results.blocks.title')}
results={searchQueries.blockHeight.results.filter(result => result.network === network)}
Copy link
Member

Choose a reason for hiding this comment

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

Eww

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you please elaborate? Is this about performance, and code readability?

Copy link
Member

Choose a reason for hiding this comment

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

We have one implementation in a higher layer for counting results, and completely different implementation in this layer for showing the results

Copy link
Member

Choose a reason for hiding this comment

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

*different implementation of counting/filtering all results

Copy link
Member

Choose a reason for hiding this comment

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

Counting and filtering logic was unified in 050a955

7e96667 and f060f9c refactored from passing in all results + filter to passing filtered results.

🎉 very readable

@csillag csillag force-pushed the csillag/network-cross-search branch from 58275d9 to dc31473 Compare May 24, 2023 13:31
@csillag
Copy link
Contributor Author

csillag commented May 24, 2023

I have changed the implementation by removing the global constant completely, but keeping global search functionality.

So "global" is now gone from both the URL and the code. I hope it's less ugly that way.

@csillag
Copy link
Contributor Author

csillag commented May 24, 2023

I have also merged the "fix theme reactivity problem" into this PR (it was too painful to maintain that branch separately), and so changed the route implementation a little bit.

@csillag csillag force-pushed the csillag/network-cross-search branch from da150e3 to 7a2d0fa Compare May 24, 2023 14:22
@csillag csillag merged commit f7ea3d1 into master May 24, 2023
4 checks passed
@csillag csillag deleted the csillag/network-cross-search branch May 24, 2023 14:25
@lukaw3d lukaw3d mentioned this pull request May 26, 2023
Merged
@lukaw3d lukaw3d mentioned this pull request Jun 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukaw3d lukaw3d lukaw3d approved these changes

@buberdds buberdds Awaiting requested review from buberdds buberdds is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@csillag @lukaw3d