Skip to content

Conversation

ryanwuer
Copy link
Contributor

@ryanwuer ryanwuer commented May 7, 2025

When searching in pool selection form, users need to clear all texts before inputing search text, which is not very convenient.

Old way:
image

New way:
image

Ant UI component library provides the function natively, https://ant-design.antgroup.com/components/select
image

@ryanwuer ryanwuer requested a review from juliusv as a code owner May 7, 2025 13:30
Signed-off-by: Ryan Wu <rongjun0821@gmail.com>
@ryanwuer ryanwuer force-pushed the optimize-target-pool-search-experience branch from 77172d8 to a810e59 Compare May 7, 2025 13:32
@ryanwuer
Copy link
Contributor Author

ryanwuer commented May 9, 2025

@juliusv Sorry, I don't mean to be rude to @ you. Could you please take a look at this PR?

@juliusv
Copy link
Member

juliusv commented May 9, 2025

@ryanwuer Oh no it's ok, thanks for pinging me.

And thanks, that's an interesting approach to handling this problem! I would challenge it though: it's a slight upside for people who want to search for something completely new, but it makes other use cases harder, such as when I just want to edit what has previously been typed. Either because I unfocused the box before I was finished (then my input text disappears) or because I want to change from e.g. serviceMonitor/monitoring/kubelet/0 to serviceMonitor/monitoring/kubelet/1, just changing a suffix in my existing search. That would get really annoying then.

Usually when I need to replace all text, I'm used to double-clicking on the input field or alternatively pressing - to select everything before typing, which then automatically overwrites things. So that feels like more of a minor hurdle to me. There is one other alternative though: adding a "clear" button to the selection box, which Mantine also natively supports, see: https://mantine.dev/core/select/#clearable. How about that? It's not a huge upside because you still need to click that first and then enter the new text, but maybe an ok middle ground?

@ryanwuer
Copy link
Contributor Author

ryanwuer commented May 12, 2025

My real using experience is when i want to change text from serviceMonitor/monitoring/kubelet/0 to serviceMonitor/monitoring/kubelet/1, there will be three steps(for a merely search operation, that could be complicated):

  1. focus the box
  2. move the cursor to the rightmost side since the width of box is not long enough
  3. edit the text

Step 2 is nearly unavoidable because the prefix serviceMonitor or podMonitor and namespace part.

Furthermore, serviceMonitor/monitoring/kubelet/1 is next to serviceMonitor/monitoring/kubelet/0, which can be selected directly from the dropdown input.
image

Adding a "clear" button to the selection box could be a workaround, but still three steps are needed to finish the search:

  1. click "clear" button
  2. focus the box
  3. input the text

@ryanwuer
Copy link
Contributor Author

@juliusv pls take another look of this issue

@juliusv
Copy link
Member

juliusv commented May 16, 2025

@ryanwuer Thanks for the reality check about the input width. I'm still not clear on what the best overall approach is. Just not making the previously inputted text available for textual editing also doesn't seem optimal, even if you need a few steps to be able to edit the suffix. Pressing the <End> key is really fast for me on my keyboard at least, maybe that's more work on some laptop keyboards where you don't have a dedicated <End> key and need to press a modifier key at the same time to jump to the end of the input.

Still, so far I still feel like just adding a "clear" button but still letting the user edit the existing text would be the best compromise for now.

@ryanwuer
Copy link
Contributor Author

ryanwuer commented May 16, 2025

@juliusv I've checked the same page in v2 version. It obviously uses a different UI library, but it clears the inputted texts.

image

For me, it's quite rare to edit the existing texts to do search. When i want to select like No.2 of a ServiceMonitor, i just type the core part of the name, like 'kubelet', and all the options are showed immediately, quite smoothly.

image

@ryanwuer ryanwuer closed this May 16, 2025
@ryanwuer ryanwuer reopened this May 16, 2025
@juliusv
Copy link
Member

juliusv commented May 16, 2025

@juliusv I've checked the same page in v2 version. It obviously uses a different UI library, but it clears the inputted texts.

Ah, interesting. Then at least it would not be a regression, although otherwise we are free to do things very differently from the old UI of course :)

For me, it's quite rare to edit the existing texts to do search. When i want to select like No.2 of a ServiceMonitor, i just type the core part of the name, like 'kubelet', and all the options are showed immediately, quite smoothly.

Ok, in the end you're a real user, while I'm not really. Ultimately I don't feel too strongly, I just wanted to make sure that we're not accidentally making things worse for other users. @Nexucis what do you think, should we just merge it and see if anyone complains?

@ryanwuer
Copy link
Contributor Author

@Nexucis hey, what do you think?

@juliusv
Copy link
Member

juliusv commented May 21, 2025

I would say if there's no opinion by @Nexucis by tomorrow, let's just merge if then. We can always change things again :)

@ryanwuer
Copy link
Contributor Author

Cool. Appreciate it.

Copy link
Member

@juliusv juliusv left a comment

Choose a reason for hiding this comment

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

Time has passed, let's merge it :)

@juliusv juliusv merged commit ef43007 into prometheus:main May 31, 2025
47 checks passed
@Nexucis
Copy link
Member

Nexucis commented Jun 12, 2025

yeah sorry for not answering. I was too busy ...

@Nexucis what do you think, should we just merge it and see if anyone complains?

I am often thinking that, specially if we think what is proposed should improve the situation for 90% of the use cases. That's good you didn't wait me to merge this one. Really cool to see these changes

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.

3 participants