-
-
Notifications
You must be signed in to change notification settings - Fork 860
Search frontend cleanup #2849
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
Search frontend cleanup #2849
Changes from all commits
7df1c0c
13fa3b2
38328b7
8d05a30
b19032a
9d2e599
62a44e3
180e374
4ea38a0
01cf969
114503b
2779825
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| import highlight from '../../common/helpers/highlight'; | ||
| import LinkButton from '../../common/components/LinkButton'; | ||
| import Link from '../../common/components/Link'; | ||
| import { SearchSource } from './Search'; | ||
| import Mithril from 'mithril'; | ||
|
|
||
| /** | ||
| * The `DiscussionsSearchSource` finds and displays discussion search results in | ||
| * the search dropdown. | ||
| */ | ||
| export default class DiscussionsSearchSource implements SearchSource { | ||
| protected results = new Map<string, unknown[]>(); | ||
|
|
||
| search(query: string) { | ||
| query = query.toLowerCase(); | ||
|
|
||
| this.results.set(query, []); | ||
|
|
||
| const params = { | ||
| filter: { q: query }, | ||
| page: { limit: 3 }, | ||
| include: 'mostRelevantPost', | ||
| }; | ||
|
|
||
| return app.store.find('discussions', params).then((results) => this.results.set(query, results)); | ||
| } | ||
|
|
||
| view(query: string): Array<Mithril.Vnode> { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe Thinking again, it's probably best left as it is.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one needs to be an array |
||
| query = query.toLowerCase(); | ||
|
|
||
| const results = (this.results.get(query) || []).map((discussion: unknown) => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of or-ing against an empty array, could we do
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need it to be a list even if it's undefined. |
||
| const mostRelevantPost = discussion.mostRelevantPost(); | ||
|
|
||
| return ( | ||
| <li className="DiscussionSearchResult" data-index={'discussions' + discussion.id()}> | ||
| <Link href={app.route.discussion(discussion, mostRelevantPost && mostRelevantPost.number())}> | ||
| <div className="DiscussionSearchResult-title">{highlight(discussion.title(), query)}</div> | ||
| {mostRelevantPost ? <div className="DiscussionSearchResult-excerpt">{highlight(mostRelevantPost.contentPlain(), query, 100)}</div> : ''} | ||
| </Link> | ||
| </li> | ||
| ); | ||
| }) as Array<Mithril.Vnode>; | ||
|
|
||
| return [ | ||
| <li className="Dropdown-header">{app.translator.trans('core.forum.search.discussions_heading')}</li>, | ||
| <li> | ||
| <LinkButton icon="fas fa-search" href={app.route('index', { q: query })}> | ||
| {app.translator.trans('core.forum.search.all_discussions_button', { query })} | ||
| </LinkButton> | ||
| </li>, | ||
| ...results, | ||
| ]; | ||
| } | ||
| } | ||
This file was deleted.
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.
Could this be better off as an
asyncfunction? It would reduce the callbacks and would automate the promise return.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.
That wouldn't be any changes other than just the modifier, right?
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.
It would mean using
awaitinstead of.then.E.g.
Would become
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.
Well yes, but we wouldn't actually want it to be blocking. So we'd still be using then.