Search frontend cleanup #2849
Search frontend cleanup #2849
Conversation
askvortsov1
force-pushed
the
as/search-frontend-cleanup
branch
from
20c5f25 to
8d05a30
Compare
|
There seems to be some conflicts with
I think we usually define them in the same file ? that's my personal preference at least, and if it's what we usually do, then it makes even more sense.
Yea a |
Moved it to Search, works fine (I was a bit worried about a circular import). I think we should reorganize all these files for v2, but not now (editor stuff could be its own folders too, for instance). |
| 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)); | ||
| } |
There was a problem hiding this comment.
Could this be better off as an async function? It would reduce the callbacks and would automate the promise return.
There was a problem hiding this comment.
That wouldn't be any changes other than just the modifier, right?
There was a problem hiding this comment.
It would mean using await instead of .then.
E.g.
fetch("example.com").then(response => doSomething(response)Would become
const response = await fetch("example.com")
doSomething(response)There was a problem hiding this comment.
Well yes, but we wouldn't actually want it to be blocking. So we'd still be using then.
| return app.store.find('discussions', params).then((results) => this.results.set(query, results)); | ||
| } | ||
|
|
||
| view(query: string): Array<Mithril.Vnode> { |
There was a problem hiding this comment.
Maybe Mithril.Children could be a cleaner type as it's used within Mithril itself.
Thinking again, it's probably best left as it is.
There was a problem hiding this comment.
This one needs to be an array
| view(query: string): Array<Mithril.Vnode> { | ||
| query = query.toLowerCase(); | ||
|
|
||
| const results = (this.results.get(query) || []).map((discussion: unknown) => { |
There was a problem hiding this comment.
Instead of or-ing against an empty array, could we do ?.map?
There was a problem hiding this comment.
We need it to be a list even if it's undefined.
| return [ | ||
| <li className="Dropdown-header">{app.translator.trans('core.forum.search.discussions_heading')}</li>, | ||
| <li> | ||
| {LinkButton.component( |
| const name = username(user); | ||
|
|
||
| const children = [highlight(name.text, query)]; | ||
| const children = [highlight(name.text as string, query)]; |
There was a problem hiding this comment.
I think it's better to put the as string at declaration.
There was a problem hiding this comment.
Can we do that to an attribute?
There was a problem hiding this comment.
Sorry, I totally missed that it was an attribute. Carry on!
|
Sorry, this was a bit long 🙈 |
askvortsov1
force-pushed
the
as/search-frontend-cleanup
branch
from
dba3669 to
9d2e599
Compare
Co-authored-by: David Wheatley <hi@davwheat.dev>
Co-authored-by: David Wheatley <hi@davwheat.dev>
Co-authored-by: David Wheatley <hi@davwheat.dev>
|
Thank you @askvortsov1 for these fixes and cleanup, especially "Fix search disappearing on page reload / direct link". I had developed a dirty workaround but your implementation is very nice and clean. I only saw this now, I think it was not written anywhere in the release notes, or was it?... |
**Fixes #2399 **
Changes proposed in this pull request:
getInitialSearchtoSearchStateReviewers should focus on:
Confirmed
composer test).