canonical / react-components

A set of components based on Vanilla Framework
https://canonical.github.io/react-components
82 stars 49 forks source link

fix(searchbox): handle Enter key in the input field to search + blur #1065

Closed lorumic closed 2 months ago

lorumic commented 3 months ago

Done

QA

Storybook

To see rendered examples of all react-components, run:

yarn start

QA in your project

from react-components run:

yarn build
npm pack

Install the resulting tarball in your project with:

yarn add <path-to-tarball>

QA steps

Percy steps

This should not produce visual changes in Percy.

webteam-app commented 3 months ago

Demo

Jenkins

demos.haus

bartaz commented 3 months ago

When is this needed?

Search box includes a submit button to perform search. And it should be placed inside a form element that performs given search. In such case ENTER key is handled natively by the browser (it performs the submit of the form).

image

I guess in apps it may be more natural to use SearchBox component outside of the context of the form that does the search. Then maybe the form itself could be part of the component?

Because the proposed solution looks a bit like a workaround to something that could work natively.

bartaz commented 3 months ago

Now I noticed the difference. In Vanilla examples the p-search-box is on <form> element. In React the root element of the SearchBox component is a <div>.

So a better solution would be to change the React component to use <form> instead and handle submit on it (that would cover both clicking on submit icon and enter key). The downside (and I think that's why it was not done this way) is that if you want to use such component INSIDE another form it would be an issue to have it's own form built-in.

lorumic commented 3 months ago

This is needed every time we have a search box to filter the results in any table / list of results. No form in that case as the action (filtering) is performed onChange. So it's just a way to align the behaviour of the click on the magnifier icon and the Enter key press. I'll leave any further clarification to the requester of this change, @acarege

bartaz commented 3 months ago

I don't think this component was meant to do filtering at all.

Search & Filter component was meant to more advanced search and filter functionality.

lorumic commented 3 months ago

Maybe I didn't express myself properly. By "filtering" I mean setting a search query and showing only the results matching that query, as in the example below:

Peek 2024-04-08 09-42

I don't think the SearchAndFilter is the right component to do this. Also, it doesn't have the magnifier icon, so it would not have the correct appearance.

bartaz commented 3 months ago

I guess we get to the point where it's more of a design UX question than implementation question.

So far the SearchBox component was meant for triggering search (when Enter/Search is hit). Not for automatic searching/filtering. The SearchAndFilter component was meant for automatic searching and filtering (why it doesn't have a search icon is a question I don't have answer to).

If neither component does what you want to do, it's something that should be brought up in design (possibly Vanilla Working Group).

lorumic commented 3 months ago

SearchAndFilter seems an overkill to do what shown in the screen capture above. To be honest, I don't see which problem the changes in this PR create. It should just work fine in every case - both inside and outside a form.

bartaz commented 3 months ago

SearchAndFilter seems an overkill to do what shown in the screen capture above. To be honest, I don't see which problem the changes in this PR create. It should just work fine in every case - both inside and outside a form.

I agree that SearchAndFilter seems overkill, I just wanted to make sure it's clarified from design point of view, because there was some effort at some point to define how search/filter should work and which component does what, and by allowing some exceptions we may move away from consistency.

As for the functionality itself, it seems that the fact that there actually is onSearch handler on SearchBox component already is allowing it not only to "submit" search relying on external form. So I guess making sure ENTER triggers it as well is an improvement. Would be good to verify how does that behave with form now. Would enter trigger the search AND submit the form? Should it cancel propagation (or default handler) so that form is not submitted if handler is triggered?

lorumic commented 3 months ago

Would be good to verify how does that behave with form now. Would enter trigger the search AND submit the form? Should it cancel propagation (or default handler) so that form is not submitted if handler is triggered?

Good point. I will test this specifically and get back to you with the result.

lorumic commented 3 months ago

@bartaz I tested the behaviour in Form component's Storybook. I added an onSubmit argument with a simple console.log to the args of the default example, then replaced the Input children with a SearchBox, and tried hitting Enter there to see if the onSubmit code got executed, but it didn't.

I'm not sure this is a good approach to test it, what do you think? Should I add an e.stopPropagation() to the keydown event handler for the Enter key just in case?

bartaz commented 3 months ago

Should I add an e.stopPropagation() to the keydown event handler for the Enter key just in case?

I'm afraid that adding that in by default will actually prevent ENTER from triggering submit of a form when it's needed. So it's probably better as is.

The only problematic scenario may be if someone has both onSearch and a surrounding form to submit. But that would be a weird case already.

bartaz commented 3 months ago

I'm not 100% sure if blur by default is a desired behaviour (what if there is some validation and you should stay in the search to fix the value?). But I guess we can verify in practice if that creates any issues.

lorumic commented 3 months ago

I'm not 100% sure if blur by default is a desired behaviour (what if there is some validation and you should stay in the search to fix the value?). But I guess we can verify in practice if that creates any issues.

What about changing the condition to:

      if (e.key === "Enter" && internalRef.current.checkValidity()) {

Would that be a good solution?

github-actions[bot] commented 2 months ago

:tada: This PR is included in version 0.51.5 :tada:

The release is available on:

Your semantic-release bot :package::rocket: