-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Search for Busy that searches within the site #1185
Conversation
* uses askSteem API for now * Fixes: #1183 * Add search reducers & search components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG
will merge after @Sekhmet approves |
src/client/helpers/apiHelpers.js
Outdated
SteemAPI.sendAsync('lookup_accounts', [name, limit]); | ||
|
||
export const getAllSearchResultPages = search => Promise.all([ | ||
fetch( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to have loop that would create array of promises instead of hardcoding those.
src/client/search/searchActions.js
Outdated
|
||
return _.reverse(_.sortBy(mergedResults, ['type', 'created'])); | ||
}) | ||
.catch(() => dispatch({ type: SEARCH_ASK_STEEM.ERROR })), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it dispatch anyway if we will remove catch
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure tbh, will it?
Fixes #1183
Changes:
We are limited on the data that askSteem has and it is sometimes not accurate, for example when it returns a user:
The users post_count, followers_count, following_count is different then the actual values... so I decided not display it