[MI-400] Advanced Search with multi table view #535
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| props | ||
| .searchFunction(searchTerm) | ||
| .then((results) => { | ||
| searchResults.value = results; | ||
| }) | ||
| .finally(() => { | ||
| searching.value = false; | ||
| }); |
There was a problem hiding this comment.
async/ await is easier to read and generally preferred when possible
| props | |
| .searchFunction(searchTerm) | |
| .then((results) => { | |
| searchResults.value = results; | |
| }) | |
| .finally(() => { | |
| searching.value = false; | |
| }); | |
| searching.value = true; // Not sure if this is needed, I don't see where it gets set to `true` | |
| searchResults.value = await props.searchFunction(searchTerm.value); | |
| searching.value = false; |
There was a problem hiding this comment.
I don't think the searching.value is needed as the caller is setting it already. Because these are components we don't want to use await as that could have unintended consequences. Hence using a .then() which resolves after the result is available.
There was a problem hiding this comment.
Would we be able to move the searching.value = false be next to the place that is being set to true? Otherwise its hard to know if that is being triggered
What are the unintended consequences of using await in components? I have used async functions within many components without issue
There was a problem hiding this comment.
Actually I just looked at it and because the searchFunction is being called in a setTimeout method it's ok to add await. But generally component libraries functions being async is a little different, because then the consumer would need to know its async which 90% of our components are not.
| onMounted(() => { | ||
| window.addEventListener('click', onClickOutside); | ||
| }); | ||
|
|
||
| onUnmounted(() => { | ||
| window.removeEventListener('click', onClickOutside); | ||
| }); |
There was a problem hiding this comment.
This doesn't seem like the Vue way of doing things as its using plain JS event handlers. This could justify a new Vue hook with reactive state attached to it. This also seems like there is a concern for accessibility since people who don't utilize a mouse can't click outside (I would think focus would control this)
Feel free to ignore this one as it may be a decent lift to do some of this but wanted note it
src/components/AdvancedSearchBar/__tests__/AdvancedSearchBar.spec.ts
Outdated
Show resolved
Hide resolved
JIRA
MI-400
Description
Our team is working on revamping the search experience and as a part the first step is to update the dropdown, here's the figma for the new dropdown view,
https://www.figma.com/design/XpIvfVpAYzgej225MXdcgL/Dashboard-v2?node-id=5198-903&t=DG2IJyhTMvrRKwdY-0
This component builds a multi table dropdown that can populate data from Campaigns, Orders, Templates etc