feat(SearchInput): add new SearchInput component #4588
Conversation
|
PF4 preview: https://patternfly-react-pr-4588.surge.sh |
mcarrano
left a comment
mcarrano
left a comment
There was a problem hiding this comment.
This looks great @nicolethoen . My only question is about what the proper keyboard interaction should be for the third example with the navigation buttons. @jessiehuff can you take a look at this and let us know what you think?
tlabaj
left a comment
tlabaj
left a comment
There was a problem hiding this comment.
I think that the 3rd example is a little confusing without context to show the whole interaction (like the document you are searching).
| /** Value of the search input */ | ||
| value?: string; | ||
| /** The count of returned search results */ | ||
| results?: string; |
There was a problem hiding this comment.
hmmm, I wonder if the name is misleading. To me it does not imply it is a count.
@tlabaj but adding more context, wouldn't that break our typical documentation and example practices? |
|
We could add a demo in the future, but I'd still be in favor of leaving the example in. This is one of the reasons it would be helpful to have example descriptions. Maybe if we changed the name of the example to something like, "Find input with multiple matches?" That would better describe the use case this is intended for. @nicolethoen @tlabaj what do you think? |
I used the same example name that was used in Core. That doesn't necessarily make it the best name, but the component is called Search Input and it already has an example with multiple matches. The example up for debate is the example with navigable options. I think navigable is the key word... |
kmcfaul
left a comment
kmcfaul
left a comment
There was a problem hiding this comment.
LGTM, I think the third example is explained enough with the 'with navigable options' description. Adding a full demo with the search input and results pages to the demo's section can be done later on.
| value?: string; | ||
| /** The number of search results returned. Either a total number of results, | ||
| * or a string representing the current result over the total number of results. i.e. "1 / 5" */ | ||
| numberOfResults?: number | string; |
There was a problem hiding this comment.
The numberOfResults name implies number is the type. Perhaps this be something more like "resultsCount", "resultsTotal", or "resultsText"?
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Looks great! Only thing I see is that when you reach the beginning or end of the navigable result list, the appropriate nav button should be disabled.
@mcoker that functionality will be defined my the consumer. They very well could have the button enabled and when clicked, search from the beginning. In that case the results string should be updated to |
Not sure exactly what you mean here @tlabaj . Are you saying that clicking "^" if you are already on the first item would re-invoke the search? I think that disabling the first and last buttons would be the right thing to do in most cases, but don't feel strongly about this. |
|
@jessiehuff are you good with the keyboard interaction on this? I wasn't sure what to expect. |
jessiehuff
left a comment
jessiehuff
left a comment
There was a problem hiding this comment.
LGTM! All functionality is keyboard accessible with the tab and enter/space keys which I think makes sense. It sounds good with VO as well. 🙂 Nice job!
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
8 participants
Closes #4521