Improvement of empty lists and 404 pages#522
Improvement of empty lists and 404 pages#522loic-sharma merged 5 commits intoloic-sharma:masterfrom sipmann:master
Conversation
src/BaGet.UI/src/SearchResults.tsx
Outdated
| </div> | ||
| ))} | ||
|
|
||
| {this._renderSearch()} |
There was a problem hiding this comment.
I'd rather avoid splitting up the HTML here if possible. What do you think of doing something like:
{(() => {
if (this.state.items.length > 0) {
// Show results
} else {
// Show empty message
}
})()}There was a problem hiding this comment.
Yeah for sure. You mean take the code from _renderSearch back to the render and do the if inside it. Right?
| {this._renderSearch()} | |
| render() { | |
| ... | |
| <div className="form-group"> | |
| <Checkbox | |
| defaultChecked={this.state.includePrerelease} | |
| onChange={this.onChangePrerelease} | |
| label="Include prerelease:" | |
| boxSide="end" | |
| /> | |
| </div> | |
| </form> | |
| // {this._renderSearch()} | |
| if (this.state.items.length > 0) { | |
| this.state.items.map( i => ... | |
| } else { ... |
There was a problem hiding this comment.
To be easier to understand. Something like this? https://gist.github.com/sipmann/9e266ca45333238ccfd9c14f033e2fa2
|
Thanks for doing this, it looks great already! I left some minor comments, I'll merge this in once you address them :) |
|
Awesome, I've just commited every suggestion :). ty for your patience |
| <div> | ||
| <h2>Oops, package not found...</h2> | ||
| <p>Could not find package '{this.id}'.</p> | ||
| <p>You can try searching on <a href={`https://www.nuget.org/packages?q=${this.id}`} target="_blank" rel="noopener noreferrer">nuget.org</a> package.</p> |
There was a problem hiding this comment.
Nice job on these rel attributes, I didn't know these existed 👍
src/BaGet.UI/src/SearchResults.tsx
Outdated
| <span>by: {value.authors}</span> | ||
|
|
||
| {(() => { | ||
| if (this.state.items.length > 0) { |
There was a problem hiding this comment.
Hm I noticed there's a small flaw with this logic. When we change the search input, we set items to an empty array, thereby causing that "Oops, nothing here..." message to flash while we load the new items. I think we should introduce a new loading property on the state object and change this line to:
| if (this.state.items.length > 0) { | |
| if (loading || this.state.items.length > 0) { |
When we start loading new items using _loadItems, we should set loading to true. Once we complete loading items, we should set loading to false.
There was a problem hiding this comment.
Yeap, totally agreed
|
Ok I left one more comment to polish up the UI as we load search elements. I can't wait to get this merged in! :) |
|
Almost done with the last suggestion. But I've found a bug at the way the search works. Fill a bug and solve in another pull request? Or solve on this one? |
|
What's the bug? If it's a one liner then we can probably add it to this change as well |
|
At line 71, we are sending the previous input value, instead of the new one. So when we type the next letter, the previous one is sent. this._loadItems(
prevProps.input,
|
|
Oh nice catch! Feel free to fix it in this PR or in another one, both approaches are fine by me |
|
Thanks for the awesome contribution :) |
Summary of the changes (in less than 80 chars)
Addresses #303