Connects: SearchBar improvements #24190
Conversation
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
The search bar looks super solid now, thanks. I'm about to push a small commit which will make the search bar responsive when there are filters selected and the window is small, let me know what you think.
Works nice, thanks, although it would be good to have some margin from the top: |
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
Overall looks fine, I'm going to approve it and fix the "Type to search something" flash before merging so that I can include these changes in the error handling branch.
I feel like the issue with filters not being taken into account in the no results copy is entangled with error handling, so it's probably better to take care of it after error handling is done rather than stepping on each other's toes right now. I'll add it to the todo tasks in the doc.
| /> | ||
| ); | ||
| const ExtraComponent = inputValue | ||
| ? attempts.every(a => isEmptySearch(a) && hasFinished(a)) && ( |
There was a problem hiding this comment.
I wish useAsync was more strict, data should be defined only for the success variant so that it's not possible to inspect data without inspecting the status first, which for me is the biggest strength of using such data structure in the first place.
| ? attempts.every(a => isEmptySearch(a) && hasFinished(a)) && ( | ||
| <NoResultsItem clusters={clustersService.getRootClusters()} /> | ||
| ) | ||
| : attempts.every(a => isEmptySearch(a)) && <TypeToSearchItem />; |
There was a problem hiding this comment.
I feel like attempts should be exported as separate values and not an array. For the TypeToSearchItem case it doesn't make sense to check the resource search attempt as we know that it'll be empty. I guess this explains why there's no status check here – if you waited for the attempt to finish here, then it wouldn't solve the issue with TypeToSearchItem appearing only after debounce.
But another way to solve it would be to check the filter search only and show the component only if data is empty.
There was a problem hiding this comment.
What's more, after trying the new version out I have to say in the current state it needs to wait for the filter search to finish. Since the filter search is done asynchronously too, there's a brief flash of "Type something to search" when opening a completely pristine search bar.
Overall the filter search probably doesn't need to go through useAsync, we could pass it through useMemo if needed and make a successful attempt. This should result in snappier UI though in the future we might need to reconsider this if the synchronous search gets too expensive and will block the UI for too long.
| * ExtraComponent is the element that is rendered above the items. | ||
| */ | ||
| NoResultsComponent?: ReactElement; | ||
| ExtraComponent?: ReactElement; |
| border: 1px solid ${props => props.theme.colors.action.hover}; | ||
| text-shadow: none; | ||
| // Prevents inner items from covering the border on rounded corners. | ||
| overflow: hidden; |
| background: ${props => props.theme.colors.levels.elevated}; | ||
| } | ||
|
|
||
| export const NonInteractiveItem = styled.div` |
There was a problem hiding this comment.
Noninteractive items cannot be active AFAIK so we could move that background part to InteractiveItem. What's the convention behind adding a dollar sign to props?
| return ''; | ||
| } | ||
| if (excludedClusters.length === 1) { | ||
| return `The cluster ${excludedClustersString} was excluded from the search because you are not logged in to it.`; |
There was a problem hiding this comment.
The filters should be taken into account when preparing the copy. Here I filter by a specific cluster to which I'm logged in to but the copy still says that an unrelated cluster was not included in the search results.
| export function isEmptySearch(attempt: Attempt<SearchAction[]>): boolean { | ||
| return !attempt.data?.length; | ||
| } |
There was a problem hiding this comment.
One guideline I'd use is that any auxiliary functions should work on the data structure wrapped by Attempt itself rather on Attempt<Something>. Wrapping the whole attempt would maybe make sense in a situation where the function receives multiple attempts and needs to produce something out of them.
But in most cases you want to check the status first and then operate directly on the attempt.data.
| // Cluster filter should not be visible if there is only one cluster | ||
| if (clusters.length === 1) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
This makes it so that we don't show any filters if your search term limits the list of clusters to just one cluster.
Could you work on that tomorrow and add a test case for this?
filtered-out-cluster-filter.mov
3 participants
This PR mostly focuses on improving the search bar styling, although I also added other small changes like improving the loading state messages or hiding cluster filters when there is only one cluster.
The main change in styling is that now the results list and the input live on the same layer, while previously only the results list was elevated.
