-
Notifications
You must be signed in to change notification settings - Fork 588
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
Improved Article Finder Searchbar with added Autocomplete #5741
Improved Article Finder Searchbar with added Autocomplete #5741
Conversation
app/assets/javascripts/components/article_finder/article_finder_search_bar.jsx
Outdated
Show resolved
Hide resolved
export const fetchArticleAutocompleteResults = async (keyword, wiki) => { | ||
const query = keywordAutocompleteGenerator(keyword); | ||
|
||
return queryUrl(mediawikiApiBase(wiki.language, wiki.project), query).then((data) => { | ||
return data.query.search.map(item => item.title); | ||
}); | ||
}; |
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.
Why not use await
if the function is marked as async
?
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.
Using await and return is redundant because then it will resolve the promise and again convert it into a promise because an async function returns a promise anyways.
See:
https://stackoverflow.com/a/44806250
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 meant in place of the then
, the return is fine.
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.
If you meant the then
I used on queryUrl
then Yes I do have used await when I called this method but I have used then
trying to abstract the API logic and its returned structure. If I remove it then every where I use it I need to know the underlying structure of the response JSON.
app/assets/javascripts/components/article_finder/article_finder_search_bar.jsx
Outdated
Show resolved
Hide resolved
Haven't tested out the PR locally, but the code looks good to me.
– Shashwat (He/him)
…On Wed, 3 Apr 2024 at 11:28 PM, Pratham Vaidya ***@***.***> wrote:
@TheTrio <https://github.com/TheTrio> @ragesoss
<https://github.com/ragesoss> this is ready for review.
—
Reply to this email directly, view it on GitHub
<#5741 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACSLJQSCZ3TTDG57EHOG3LTY3Q7K7AVCNFSM6AAAAABFQPBGUOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZVGI2TEOBSGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
|
Nice, this works pretty smoothly! |
thanks much for the code review @TheTrio ! |
What this PR does
Closes #649
This PR improves the Article Finder Searchbar UI and adds autocomplete functionality to the finder.
Screenshots
Before:
After:
screen-recording-2024-03-31-19.56.webm