patternfly-react
patternfly-react copied to clipboard
patternfly
feat(context selector): replaced input group with search input
What: Closes #7208
I've removed the input group and replaced it with search input. In the process, I've removed the Enter key press functionality because it was already implemented by the SearchInput and I've reused the aria-label, placeholder, onChange and value props.
Preview: https://patternfly-react-pr-8045.surge.sh
A11y report: https://patternfly-react-pr-8045-a11y.surge.sh
@tlabaj I've updated the branch for this PR. Also, I noticed some snapshot tests related to context selector are failing. Once my changes are approved, should I update the snapshots? (my terminal is suggesting it: "2 snapshots failed from 1 test suite. Inspect your code changes or run yarn test -u to update them")
@tlabaj I've updated the branch for this PR. Also, I noticed some snapshot tests related to context selector are failing. Once my changes are approved, should I update the snapshots? (my terminal is suggesting it: "2 snapshots failed from 1 test suite. Inspect your code changes or run
yarn test -uto update them")
@gefgu you should update the snapshots before PR is reviewed. Reviewers will look at that too.
This looks good, just have a question around a specific behavior. I expect to have to hit enter/click on the arrow button to submit my search as you have here, but I don't expect to have to do that when I want to reset the list of menu items after submitting a search. In other words, if I input something, press enter to search, and then delete my input to reset to the original list of menu items, I don't expect to have to hit enter to submit my "empty" search. Instead, I would expect the menu items to reset on their own when I delete the input.
Again, I'm not sure whether that's just how these things go and if you press enter to search on something you also have to press enter to search on nothing, but thought I'd bring it up for discussion!
https://user-images.githubusercontent.com/51165119/194410385-751cd283-80df-4c3a-9bf1-e7c7593bc8f2.mov
@tlabaj Thanks for the feedback! I updated the snapshots.
@mmenestr your suggestion is great! I believe that when I delete my input in the search, the filter should reset because I'm starting my search again. It's a nice point of view.
Regarding its implementation, I think this logic would need to be passed as a prop to the Context Selector component. In other words, it would be passed on the onChange prop. So, I think it is a good idea to build a demo using this logic. I built this example, and if you agree with my opinion about creating a demo about this logic, I will add the code to this Pull Request or to another one (if is better to keep the changes separated). Have a look: (in this example, I'm not pressing enter)
https://user-images.githubusercontent.com/53129852/194443259-12e5ccf7-6fd4-4b3d-ba54-b6099d4793e6.mp4
@gefgu I personally think that looks great!! @tlabaj I'll let you make the call. Idk if we should show demos for both mechanism vs just the one though - I personally do prefer this updated version from a usability standpoint though
@mmenestr my $0.02 about the default behavior - when hitting enter or clicking the search button is what updates the menu (versus the menu updating as you type/change the text in the input), I would expect that in order to update the menu based on what's in the search input, I'd have to hit enter or click the search button regardless of what's in it. Otherwise, I think it's inconsistent and potentially unexpected, and mixing updating the menu onChange vs onSubmit.
I know we have a search button on the current context selector, but I'm curious why we need a search button there? The menu filter example, composable app launcher demo, and the context selector guidelines don't have one, though I realize some of those may be out of date.
I agree with what @mcoker said. I would expect to menu to update when input is submitted (clicked search button/ enter pressed). Regardless of value. As far as having the search button, the previous implementation of the context selector had one so I would expect one so I assume that is why there is one here. @mmenestr what was your expectation her from a design perspective?
@mcoker @tlabaj All great points! I think we should actually match the core implementation then, and just do a basic search input instead of a search input with the submit button. The only reason there is a submit button now is because it used to be an input group with a submit - but I'm not sure what the design decision was behind that... but I personally favor a search where you don't have to press enter.
@mcoker @tlabaj All great points! I think we should actually match the core implementation then, and just do a basic search input instead of a search input with the submit button. The only reason there is a submit button now is because it used to be an input group with a submit - but I'm not sure what the design decision was behind that... but I personally favor a search where you don't have to press enter.
@mmenestr in the context of how the context selector was designed initially, it made sense for the search to happen once enter was pressed or the button was clicked. Otherwise, what was the purpose of the button.
but I personally favor a search where you don't have to press enter.
@mmenestr Are you saying you want the menu list to update every time a character is entered (similar to typeahead select)?
Another question @mcoker, should we have a clear button like we had in previous implementation?
Another question @mcoker, should we have a clear button like we had in previous implementation?
@tlabaj I think it's probably useful. I don't see any design guidelines around when to use it, but most of the screenshots in the guidelines include it. Though FWIW the close button there currently is created by the browser because it's an <input type="search"> , and it's only there in some browsers. For example, there is no clear button in Firefox. https://codepen.io/mcoker/pen/gOzQdZg
@tlabaj that is what I'm saying but if you disagree that's ok! I don't feel super strongly about it. We can just have it as it was originally designed, and make the user press enter to refresh the list no matter what!
@gefgu are you still interested on working on this PR?
Unfortunately, I can't keep working on it.
@tlabaj I had to update the SearchInput component to resolve the submit button issue you had mentioned. Based on the convo in this PR regarding submitting an empty query, that seemed to make the most sense, though.
@mmenestr Eric has updated the Search input so that is is only disabled if the isDisabled prop was set. It used to be disabled if there was no search value. Based on this comment I made.
Do you have an objection to that update. In this use case it does not make sense for the button to be disabled when the field is cleared.