Search: add component #424
Conversation
I don't see a Regarding descriptions in general, we use the same descriptions as found in the SUI docs. If a prop does not exist in the SUI docs, we try to maintain the same style when describing it.
Stardust (Semantic-UI-React) excludes all styling. We create React components that render valid SUI markup. This way, SUI CSS will always work, including any user's customized themes. If we'd like to change the CSS, we'll need to do this on SUI core.
Sorry, I don't quite follow here. Here's my intuition on this component. Let me know if we're on the page. Intuitively, I'd expect a component like to provide a callback for search query changes. I'd take that query and use my own method of searching (whether API or some local search). I'd pass the results as a prop back to the search. The search then would render the results in the results list. The user would then have keyboard navigation (up, down, enter) to choose a result. They could also simply click an item. There would be a callback fired on when the user chose an item (enter or click). That action would likely close the menu. I'd then take the user's selection and do what I may. Does this make sense?
Agreed.
In the React paradigm, I think most of these are best left to the developer choice of implementation. I think the showNoResults prop and the ability to customize it makes sense. Let' see how the component plays out and decide.
Agreed again. API SpecPer our contributing guidelines, here's a start on the API spec. I think it really helps to hash through the code for these things. StandardI propose we always use an Input component over a HTML <Search /><div class="ui search">
<div class="ui icon input">
<input class="prompt" type="text">
<i class="search icon"></i>
</div>
<div class="results"></div>
</div>
CategoryThis would be used to choose the correct result renderer callback. Also, it would influence the filter function for local searches. See below for more details on callbacks and local searches. const source = {
category1: {
name: 'Category 1',
results: [{}, ...{}],
},
category2: {
name: 'Category 2',
results: [{}, ...{}],
}
}
<Search category source={source} /><div class="ui category search">
<div class="ui icon input">
<input class="prompt" type="text">
<i class="search icon"></i>
</div>
<div class="results"></div>
</div>Loading<Search loading /><div class="ui loading search">
<div class="ui icon input">
<input class="prompt" type="text">
<i class="search icon"></i>
</div>
<div class="results"></div>
</div>Fluid<Search fluid /><div class="ui fluid category search">
<div class="ui icon input">
<input class="prompt" type="text">
<i class="search icon"></i>
</div>
<div class="results"></div>
</div>Aligned<Search aligned='left|right' /><div class="ui <left|right> aligned category search">
<div class="ui icon input">
<input class="prompt" type="text">
<i class="search icon"></i>
</div>
<div class="results"></div>
</div>CallbacksLoosely based on our Dropdown callbacks and the SUI Search callbacks.
Formatting ResultsOur Table accepts an array of objects (table row data). We provided Renderer callbacks would be called with each result/category and return JSX for each. const renderCategory = (category) => (/* ... */)
const renderResult = (result) => (/* ... */)
<Search
categoryRenderer={this.renderCategory} // called if category={true}
standardRenderer={this.renderResult} // called otherwise
/>Internal vs External Searching?This is an unresolved area for me. Should we/how would we handle searching within the component vs outside of the component? If you're using the search change event to retrieve new results and passing those in via props, then there is no way/reason for the search to filter the items in the list internally. Search changes would trigger new external searches and new results for the list. So, there are two mutually exclusive use cases for this component. Local searchExcept for I think this is also where we could address some of the search shortcomings raised for the Dropdown search. This is where we could provide an optional const source = [
{ first: 'John', last: 'Doe', age: 58, city: 'Nashville' },
{ first: 'Jane', last: 'Doe', age: 29, city: 'Seattle' },
// {...}
]
<Search
source={source} // data to search
searchFullText // options for searching the results
searchFields={['first', 'last']} // which fields to search in the results
/>Remote searchI think this is probably the more common React use case. All searching/filter would be handled outside of the component. Whether those were API calls, or simple filter functions. <Search
source={this.state.searchResults}
onSearchChange={this.handleSearch}
/>It may help to call this prop That's a lot to take in, thoughts? |
jeffcarbs
force-pushed
the
feature/search
branch
from
25e9033 to
e54e008
Compare
Current coverage is 98.73% (diff: 99.09%)@@ master #424 diff @@
==========================================
Files 102 106 +4
Lines 1524 1744 +220
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 1504 1722 +218
- Misses 20 22 +2
Partials 0 0
|
|
Update: Thanks for the feedback! I'll take a second pass at some point in the next few days. @levithomason - I'll go through your comment in more detail, but wanted to show you something I just added for the search examples and get your thoughts. I'm thinking of it as a "Component Explorer" Full commit here (TechnologyAdvice/stardust@e54e008) but going to paste in the message:
Thoughts? |
|
Hahaha, this is awesome! We're very much on the same page. I've got new docs in mind once we hit v1 components. This is one of the reasons we are pulling all enum props into the static This is not the first time I've gotten a doc suggestion related to work I've kept in the background. Due to this, I'm bringing that work forward now. I've pushed it to https://github.com/TechnologyAdvice/stardust/pull/427, will leave all future communication on docs to that PR. |
jeffcarbs
force-pushed
the
feature/search
branch
2 times, most recently
from
76599cf to
1e7d83a
Compare
|
Made the updates discussed above. I also updated it to use the new I started writing more specs but got hung up a little in terms of how exactly to test everything. Also, I'm not sure I understand this failing spec: It seems to be implying that the root element of the Then it was saying the same thing but for the |
✖ handles events transparently
<Search onFocus={handleFocus} />
^ was not called on "focus".You may need to hoist your event handlers up to the root element.This is one of the This failure indicates that the So, if a user does this then focuses the Search, the handler is never called. const handleFocus = () => console.log('Search was focused')
<Search onFocus={handleFocus} />I'll take a look at the component and give some suggestions. |
|
Indeed, To the root element: The component will continue to work due to event bubbling, but so will the conformance tests as firing a |
|
These comments were edited after my first two responses, so I'll respond now.
You are correct on all counts here. It is suggesting to hoist them, and the nested input is actually what is firing the change event. The awkwardness here is warranted and is due to the limitations of the current common test for events. I'll try to explain that. The decision to hoist handlers was made for a few reasons:
Admittedly, this is not optimal. There are a number of ways we could go:
1 is not idea, 2 adds complexity, and 3 IMHO is the most correct approach but also the most costly time wise. |
jeffcarbs
force-pushed
the
feature/search
branch
3 times, most recently
from
c897054 to
0d5c987
Compare
|
Added specs and squashed commits. I think this is ready for review. The specs are failing on circle because phantomjs crashed, which seemed like an intermittent issue unrelated to this. They're passing locally for me - could you maybe pull this branch down and see if it's something introduced here or just a circle issue? One thing this didn't address was the result formatting but I think that could be handled via the I'm eager to start using this component in my app so it'd be great to be able to get this to the finish line :) |
|
There's an intermittent failure issue with webpack and react-highlightjs. A rebuild seems to have passed. Though, looks like we need more coverage. I suggest using the codecov browser extension to help find missing coverage. It augments the github interface. PRs show coverage highlights right on the diff. Also,you can open the index.html in the coverage/ directory to drill into coverage info. It is updated after every test run. I'm out today, but will review this as soon as I can. |
jeffcarbs
force-pushed
the
feature/search
branch
3 times, most recently
from
6f71c1c to
83f74b3
Compare
|
Ok made some updates:
|
jeffcarbs
force-pushed
the
feature/search
branch
from
83f74b3 to
9c5cdf8
Compare
|
Hm, seems it is crashing on the Search tests. The last test to complete, just before the crash, is: I've also retried the build without cache to ensure it has a complete run. Lastly, it crashes locally for me as well :/ Just after Item "has sub component ItemContent". Not sure what is going on there, though, master doesn't crash nor other branches that I know of. There could be something in this PR causing it. I'll have time tomorrow morning to check into these things. |
|
Added this branch locally to my app and quickly realized
I think I'm gonna take your advice and use the Currently: // Search.Category
<ElementType {...rest} className={classes}>
<div className='name'>{name}</div>
{children /* Array of Search.Result */}
</ElementType>
// Search.Result
<ElementType {...rest} className={classes} onClick={handleClick}>
{image && <div className='image'>{createImg(image)}</div>}
<div className='content'>
{price && <div className='price'>{price}</div>}
{title && <div className='title'>{title}</div>}
{description && <div className='description'>{description}</div>}
</div>
</ElementType>I'm thinking just move the internal stuff to a defaultRenderer and allow passing // Search.Category
const defaultRenderer = () => (
<div className='name'>{name}</div>
)
// Within render()
<ElementType {...rest} className={classes}>
{props.renderer ? props.renderer(props) : defaultRenderer()}
{children /* Array of Search.Result */}
</ElementType>
// Search.Result
const defaultRenderer = () => (
{image && <div className='image'>{createImg(image)}</div>}
<div className='content'>
{price && <div className='price'>{price}</div>}
{title && <div className='title'>{title}</div>}
{description && <div className='description'>{description}</div>}
</div>
)
// Within render()
<ElementType {...rest} className={classes} onClick={handleClick}>
{props.renderer ? props.renderer(props) : defaultRenderer()}
</ElementType>This would prevent things like rendering categories/results in different orders which would mess up the component's functionality. Basically we render the "shell" of the results and enable people to customize the internals. I'll give this a shot and see how it works in my app, but would love any feedback if you have any. |
jeffcarbs
force-pushed
the
feature/search
branch
from
fef1e4d to
68a7c81
Compare
|
Found the bug! At some point I had added an Looks like all the checks are passing now... finally haha. I implemented the renderers mostly as I had described. The one slight change was keeping the |
|
I'm traveling today, hope to review this at the airport! |
| name: 'Search', | ||
| type: META.TYPES.MODULE, | ||
| props: { | ||
| size: ['mini', 'small', 'large', 'big', 'huge', 'massive'], |
There was a problem hiding this comment.
Let's use the SUI constant here for the sizes. It looks like all except medium are supported. You can reference the Rail for example:
https://github.com/TechnologyAdvice/stardust/blob/master/src/elements/Rail/Rail.js#L41
|
There's an odd behavior when entering the initial search. All results are shown, then after the timeout, only the matching result or the no results message is shown. EDIT It looks like when clearing the search query, all results are added to state, then the initial next query change shows them all before the search is actually performed in the timeout. Here's the state after clearing the query: {
"isLoading": false,
"results": [
{
"title": "Williamson and Sons",
"description": "Future-proofed 4th generation monitoring",
"image": "https://s3.amazonaws.com/uifaces/faces/twitter/mrxloka/128.jpg",
"price": "$55.00"
},
{
"title": "Berge Inc",
"description": "Virtual reciprocal groupware",
"image": "https://s3.amazonaws.com/uifaces/faces/twitter/rtyukmaev/128.jpg",
"price": "$46.00"
},
{
"title": "Heathcote - Kassulke",
"description": "Self-enabling discrete orchestration",
"image": "https://s3.amazonaws.com/uifaces/faces/twitter/nacho/128.jpg",
"price": "$5.00"
},
{
"title": "Bernhard Inc",
"description": "Customizable 3rd generation methodology",
"image": "https://s3.amazonaws.com/uifaces/faces/twitter/jonkspr/128.jpg",
"price": "$75.00"
},
{
"title": "Keeling, Kihn and Schinner",
"description": "Cross-platform mobile info-mediaries",
"image": "https://s3.amazonaws.com/uifaces/faces/twitter/buleswapnil/128.jpg",
"price": "$37.00"
}
],
"value": ""
} |
| const re = new RegExp(_.escapeRegExp(value), 'i') | ||
|
|
||
| const isMatch = (result) => | ||
| _.some(searchFields, (field) => result[field] && re.test(result[field])) |
There was a problem hiding this comment.
Since the regex will correctly pass/fail against any field value, the extra result[field] is not necessary.
|
Overall, I'm not sure it makes sense to any local search/filtering in this component. Wouldn't this be left to the API's search that you are searching? I don't have the depth of visibility you do here, but my question is, how is it possible to reconcile a query if it is both applied internally to the objects and externally against some API? It seems it can only either filter the list locally, or else be used externally by some other search mechanism (an API). I'm thinking if I searched google for 'videos', i'd expect a results list of videos. Those results would be what ever that engine decided to return. The component displaying those results shouldn't again "search" the search results. LMK if I'm totally missing something here. |
|
|
||
| return _.map(categories, (category, name, index) => { | ||
| const categoryProps = { | ||
| key: `${category.name}-${index}`, |
There was a problem hiding this comment.
We're removing use of index or any other dynamically changing data in key props. It is an antipattern. See https://github.com/TechnologyAdvice/stardust/pull/452 for more.
We are instead using any part of the object we can expect to be unique, like category name here. Additionally, just in case that fails, then we use the user's childKey prop. We accept a childKey prop instead of key as the key prop is removed by React. In order to pass in a key value, it must be on a different prop name.
This then would look something like this:
return _.map(categories, ({childKey, ...category}, name, index) => {
const categoryProps = {
key: childKey || category.name,
// ...
|
Re: local search: I implemented it because it's something SUI supports, however I'm fine with not doing things that don't make sense in the React world. It would simplify this component quite a bit. I could see a case being made that it gives the developer an option between a controlled or uncontrolled component. // Uncontrolled
import longListOfItems from 'lib/data'
const search = () => <Search source={longListOfItems} />// Controlled
class Search extends Component {
handleChange = (e, result) => this.setState({ value: result.title })
handleSearchChange = (e, value) => {
this.setState({ isLoading: true, value })
// this.makeApiRequest
}
render() {
const { isLoading, value, results } = this.state
return (
<Search
loading={isLoading}
onChange={this.handleChange}
onSearchChange={this.handleSearchChange}
results={results}
value={value}
/>
)
}I don't personally have a use-case for the uncontrolled input since I'm going to be relying on a server to filter the data via a query param. Re: The odd behavior within the search during loading: I think that's more of an issue with the example. It'll be up to the developer to determine which results to show when. In that example, when you start typing it uses the results from the previous query until new ones are fetched. I can update the example so it wipes the results once you go lower than the minCharacters. |
| * | ||
| * @returns {string} local url | ||
| */ | ||
| const safeImageUrl = () => '/base/test/images/logo.png' |
There was a problem hiding this comment.
I think I'd prefer to simplify things for images instead. Since we don't need the images to actually render, how about just using foo.png instead of real faker urls or the safeImageUrl?
There was a problem hiding this comment.
That doesn't work. Looking for the non-existent URL returns a 404 and crashes phantomjs:
active item
✔ defaults to no result active
✔ defaults to the first item with selectFirstResult
✔ moves down on arrow down when open
19 09 2016 10:41:45.317:WARN [web-server]: 404: /foo.png
✔ moves up on arrow up when open
✔ skips over items filtered by search
✔ scrolls the selected item into view
✔ closes the menu
✔ uses custom renderer
category
✔ defaults to the first item with selectFirstResult
✔ moves down on arrow down when open
19 09 2016 10:41:45.380:ERROR [launcher]: PhantomJS crashed.
19 09 2016 10:41:45.382:ERROR [launcher]: PhantomJS failed 2 times (crashed). Giving up.
There was a problem hiding this comment.
hm, how about http://placehold.it/100 or http://unsplash.it/100?
There was a problem hiding this comment.
I could try, although it's still going to go out to the internet to download an image which I'm not sure is best practice in a test suite.
There was a problem hiding this comment.
Agreed, it's definitely not ideal. I haven't checked the Karma config for this, but I wonder if we could tell it to ignore foo.png. Perhaps, serve: false would prevent the 404 from happening?
I'll defer to you on this since you've got the deeper perspective. LMK what we should do here.
There was a problem hiding this comment.
OK, I removed the safeImageUrl stuff and updated karma to load foo.png locally.
jeffcarbs
force-pushed
the
feature/search
branch
3 times, most recently
from
3372772 to
b7246db
Compare
|
@levithomason - updated based on the most recent feedback. Haven't removed local search just yet, I wanted to get your thoughts on my last comment about that. Otherwise, lmk what else there is to do |
|
Regarding local search. It seems that is what differentiates the Search from the Dropdown in the React paradigm. If you already have items, you're probably "selecting" one of them. If you need to fetch the items from some remote location, based on the query, you are I think that is all for this PR. In the future, I'd like to explore a base component that handles the commonalities with the Dropdown. Though, I think that is beyond scope for this PR. |
YES! I actually started working on this but struggled with the appropriate level of abstraction. I'll open a new issue with some general thoughts and roadblocks I ran into. |
|
Ready for a final here? |
|
Not sure if "reactions" translate to notifications or anything, but yea it's ready for a final review 👍 |
|
Thanks, just saw that :) For the record, no notifications were fired for the "reaction". |
|
One request for the docs before we ship, can we update this.setState({ isLoading: true, value, results: [] }) |
* Use Input, clean up unused props * Category search * Update Search parts to use ElementType * Add specs * Add examples to mirror semantic-ui.com * Adds custom renderers for category and result Update for tests: Whitelist foo.png in karma so it can be served locally.
jeffcarbs
force-pushed
the
feature/search
branch
from
df82617 to
43a356b
Compare
|
I updated the examples to clear the results when the search query was cleared but after the timeout, see: https://github.com/TechnologyAdvice/stardust/pull/424/files#diff-a361c4ea0f5d24c0ed1827c86f6313acR26, which I think should be sufficient. Clearing the results immediately on change makes it kinda weird because "no results" is shown between each query. Let me know if that's cool. Just rebased and squashed. |
|
Thanks much! Released in |
I modeled this off of the
Dropdowncomponent since there were a lot of similarities. There are some open questions and things left to do:Questions:
selectionclass used for inSearch? It seems to just makes the border less round. I've left it in but didn't know what to put for the description.Remaining work
There are some more options and functionality that the SUI search component provides that I haven't done yet. I'm not sure what the stance is on ensuring 1-1 feature functionality with the jquery version. Here are the settings from the SUI search docs
Done or makes sense to do IMHO:
Not sure:
Don't think it makes sense to do:
onSearchChange.Other TODOs
cc #195