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

Network proposal votes: add search by validator name #1357

Merged
merged 5 commits into from
May 16, 2024

Conversation

csillag
Copy link
Contributor

@csillag csillag commented Mar 31, 2024

This adds the search feature for the network proposal vote list.

@csillag csillag force-pushed the csillag/proposal-show-votes-simplified branch from 2d971f4 to c6913a8 Compare March 31, 2024 22:11
@csillag csillag changed the title Proposal votes: add search by validator name Network proposal votes: add search by validator name Mar 31, 2024
@csillag csillag force-pushed the csillag/proposal-show-votes-with-search branch from 2f3359a to 5ceb728 Compare March 31, 2024 22:14
@csillag csillag changed the base branch from csillag/proposal-show-votes-simplified to master March 31, 2024 22:17
@csillag csillag force-pushed the csillag/proposal-show-votes-with-search branch from 5ceb728 to f4a54ba Compare May 9, 2024 10:12
Copy link

github-actions bot commented May 9, 2024

Deployed to Cloudflare Pages

Latest commit: 9dc098cdaa3ef6b472c421779bd01ebca2304fb6
Status:✅ Deploy successful!
Preview URL: https://eb6f4240.oasis-explorer.pages.dev

@csillag csillag force-pushed the csillag/proposal-show-votes-with-search branch from f4a54ba to 832d974 Compare May 9, 2024 10:49
@csillag csillag force-pushed the csillag/proposal-show-votes-with-search branch 7 times, most recently from 9b2a36a to b7a4710 Compare May 9, 2024 14:12
@csillag csillag marked this pull request as ready for review May 9, 2024 14:19
@csillag csillag requested review from lukaw3d, buberdds and lubej as code owners May 9, 2024 14:19
Comment on lines 26 to 30
export function useTypedSearchParam<T = string>(
paramName: string,
defaultValue: T,
defaultSetterOptions: UrlSettingOptions = {},
): [T, SetterFunction<T>] {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of T = string, lets do something like

const get = <T extends keyof StorageType>(key: T): StorageType[T] | undefined => {
in the future

@csillag csillag force-pushed the csillag/proposal-show-votes-with-search branch 7 times, most recently from d0a7a79 to 0ca0f1c Compare May 13, 2024 15:08
@csillag
Copy link
Contributor Author

csillag commented May 16, 2024

Also search looks weird:

image

* shadow doesn't look right

* message bellow(its background) is somehow wider than the search box itself

This has been extensively reviewed by @donouwens . Everything has been adjusted as he requested. I am open to changes though, if there are any specific requests.

@csillag csillag force-pushed the csillag/proposal-show-votes-with-search branch from 92fa16d to 4c5ddca Compare May 16, 2024 07:54
@donouwens
Copy link
Collaborator

Highlight part looks kind of weird, I was searching for "mainnet-upgrade" and it highlights half of "-" as well?

When we have first introduced the highlights, someone (probably @donouwens ?) has requested me to increase the padding, so we always highlight half a character extra, on both sides. It's all in the CSS.

Should we adjust it, @donouwens ?

I don't recall this tbh. It does not seem logical to highlight sections that are not part of the search query

@donouwens
Copy link
Collaborator

Should we add proposal example in the suggestion bar bellow the search bar:

Good question. Technically it would make sense, but it's a very small niche, most users don't care about it. I think it would just draw focus away from more important things, like txs, accounts, blocks, etc.

But I defer this to @donouwens , too.

I would say the search bar here is self-explanatory. Users are 'deep' into the Explorer so we can reasonably expect them to know what they'd be searching for (unlike with the main search bar of the Explorer). My vote would be to not include suggestions.

@donouwens
Copy link
Collaborator

Also search looks weird:
image

* shadow doesn't look right

* message bellow(its background) is somehow wider than the search box itself

This has been extensively reviewed by @donouwens . Everything has been adjusted as he requested. I am open to changes though, if there are any specific requests.

The width and radius of the dropdown do not seem to match in the screenshot indeed. Could you match the width and radius to match the input field (visually), please?

@csillag
Copy link
Contributor Author

csillag commented May 16, 2024

It does not seem logical to highlight sections that are not part of the search query

OK, reduced padding from 4px to 2px.
Before:
kép
After:
kép

@csillag
Copy link
Contributor Author

csillag commented May 16, 2024

I would say the search bar here is self-explanatory. Users are 'deep' into the Explorer so we can reasonably expect them to know what they'd be searching for (unlike with the main search bar of the Explorer). My vote would be to not include suggestions.

@donouwens , I think @lubej was actually speaking about the main search bar of the Explorer. Right?

(However, Don, your interpretation is also understandable, since PR is about adding another search bar besides that one...)

@donouwens
Copy link
Collaborator

I would say the search bar here is self-explanatory. Users are 'deep' into the Explorer so we can reasonably expect them to know what they'd be searching for (unlike with the main search bar of the Explorer). My vote would be to not include suggestions.

@donouwens , I think @lubej was actually speaking about the main search bar of the Explorer. Right?

(However, Don, your interpretation is also understandable, since PR is about adding another search bar besides that one...)

Ahhh, misinterpreted the message indeed. Sorry. We can add it, on desktop it's no problem, but mobile might make it tricky with the width (I would not want this to go over 2 lines tbh). Send you design on Slack @csillag

@csillag
Copy link
Contributor Author

csillag commented May 16, 2024

The width and radius of the dropdown do not seem to match in the screenshot indeed. Could you match the width and radius to match the input field (visually), please?

OK, updated to:
kép

@csillag csillag force-pushed the csillag/proposal-show-votes-with-search branch from b3ac251 to aec6efc Compare May 16, 2024 09:36
@csillag
Copy link
Contributor Author

csillag commented May 16, 2024

We can add it, on desktop it's no problem, but mobile might make it tricky with the width (I would not want this to go over 2 lines tbh).

On further discussion, @donouwens 's verdict is:

I [...] prefer to keep it out. [...]
do not incorporate the suggestion in the dropdown

So I guess we don't need to do anything about this, for now.

@csillag csillag requested review from lukaw3d and lubej May 16, 2024 09:39
Copy link
Collaborator

@lubej lubej left a comment

Choose a reason for hiding this comment

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

Apart from few small nits, LGTM

src/app/components/Search/TableSearchBar.tsx Outdated Show resolved Hide resolved
src/app/components/Search/TableSearchBar.tsx Show resolved Hide resolved
src/app/components/Search/TableSearchBar.tsx Show resolved Hide resolved
src/app/components/Search/TableSearchBar.tsx Show resolved Hide resolved
src/app/utils/vote.ts Outdated Show resolved Hide resolved
@csillag csillag force-pushed the csillag/proposal-show-votes-with-search branch 2 times, most recently from 953f2ce to 3111f56 Compare May 16, 2024 12:28
@csillag csillag requested a review from lukaw3d May 16, 2024 16:19
src/app/pages/ProposalDetailsPage/ProposalVotesCard.tsx Outdated Show resolved Hide resolved
src/app/pages/ProposalDetailsPage/hooks.ts Outdated Show resolved Hide resolved
@csillag csillag force-pushed the csillag/proposal-show-votes-with-search branch from 28b2481 to c4dd5d1 Compare May 16, 2024 16:45
Also, handle situation when there are 0 votes
@csillag csillag force-pushed the csillag/proposal-show-votes-with-search branch from c4dd5d1 to 9dc098c Compare May 16, 2024 16:49
@csillag csillag enabled auto-merge May 16, 2024 16:49
@csillag csillag merged commit 1dccca2 into master May 16, 2024
8 checks passed
@csillag csillag deleted the csillag/proposal-show-votes-with-search branch May 16, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants