Speak-32 Add search algorithm #195
Conversation
There was a problem hiding this comment.
Hey, thanks for adding this change!
We'd like to stick with camelCase and not introduce kebab-case, so please rename the no-results file as well as update the translation key.
Can you please write a little more details on your testing steps. Manual is fine, but detailing a few test cases you used helps us QA your changes more quickly. 🤠
|
@ann-kilzer resolved kebab conflict; added more info for QA. |
There was a problem hiding this comment.
This is looking great and it's going to be so helpful to be able to search. Pardon my delayed review.
I'd like to request a few changes based on things I noticed while testing:
- Since we only search by prefecture, let's change the Label "City, Prefecture, or Region" to "Prefecture."
2). I notice that when there is only one page of results, it appears possible to page to see more results. For example, search for "Blockchain" and see the following:
It might be cool to allow searching on multiple Topics, using "OR" logic, but that's fine to happen in a later PR. Thanks again!
web/src/views/FindSpeaker.vue
Outdated
| @@ -86,21 +90,24 @@ export default { | |||
| Search, | |||
| SpeakerCard, | |||
| PaginationRow, | |||
| NoResults, | |||
| }, | |||
| data() { | |||
There was a problem hiding this comment.
(minor comment) There seem to be different opinions on which syntax is preferable. Actually this way looks pretty nice, though some of the VS Code plugins I use feel differently. 😄
web/src/i18n/locales/ja.json
Outdated
| @@ -48,7 +48,9 @@ | |||
| "whyQ": "なぜ重要なのでしょうか？" | |||
| }, | |||
| "findSpeaker": { | |||
| "title": "スピーカーを探す" | |||
| "title": "スピーカーを探す", | |||
| "pronouns": "Pronouns: {0}", | |||
There was a problem hiding this comment.
Please leave translation keys blank for ja.json for now, as we have the fallback locale of English. Thanks!
|
@ann-kilzer updated PR |
There was a problem hiding this comment.
Thanks for the updates. It looks like the pagination issue has been fixed. 👍🏻
Almost ready to approve, but please address my comment and update with the latest master.
Please don't use force-push when contributing to this project, as it breaks the comment history within git, and can also cause issues if other people are working on the same branch. I recommend using merges. See our contributing guidelines
| @@ -74,17 +72,15 @@ export default { | |||
| this.$db('Location') | |||
| .select({ | |||
| view: 'All', | |||
| fields: ['prefecture', 'region'], | |||
| fields: ['prefecture'], | |||
There was a problem hiding this comment.
🤔 I just noticed that the region column is in the DB, but there are no values. Probably best to add it later and fill out the DB.
| return this.selectedSpeaker.get('name_ja') || this.selectedSpeaker.get('name_en') || ''; | ||
| } | ||
| return this.selectedSpeaker.get('name_en') || ''; | ||
| if (!this.selectedSpeaker) return ''; |
There was a problem hiding this comment.
Can we keep this PR focused on search and avoid making other refactors? It gets confusing to follow the changes.
|
Hi @tuul-wq ! Any plans to come back to this PR? |
There was a problem hiding this comment.
Let's just get this in, it's going to make the site sooo much better. If we want to make follow ups, we can do it in future PRs. Thanks again @tuul-wq for the great work!
Resolves #32
Description of changes in this PR
Enabled
Searchcomponent which has 2 autocomplete fields with a prepopulated dropdown.Fields can be cleared with a cross icon. In case no speakers were found we show
NoResultscomponent with corresponding text (translation required). Also adjusted spinner center align as well as margin values to be similar withNoResultscomponent to eliminate footer jumping. MovedPaginationRowin title row, to remove jumping between search form and pagination widget.How do the reviewers QA this?
@changeevent).