fix(quick-search): Quick Search Navigation

[shahzadaziz]

This fix adds logic to skip keyboard focus handlers for New Quick Search components

[JChan106]

Can we update this comment, maybe put the comments directly above each case in isTargetQuickSearch

[JChan106]

lgtm

[TylerGauntlett]

🔧 this function feels hard to follow as there are multiple targets all && and || together. Can we use guard statements or variables to hold context as to what the selectors are?

[shahzadaziz]

@TylerGauntlett good suggestion.

[TylerGauntlett]

❓ optional chaining offer any value here? className?.includes

[shahzadaziz]

Not sure it does since we are checking for className.

[TylerGauntlett]

I guess i mean dropping the className check and using optional chaining in both

[TylerGauntlett]

🔧 might be worth adding comments over each of these if blocks detailing exactly what its targeting. These class names might be a easy enough to put together with the comment overtop the function but the dataset strategy below becomes much harder

[TylerGauntlett]

Something like this

        if (!event.target) {
            return false;
        }

        const { className, dataset } = event.target;

        // QuickSearch Recent Item
        if (className?.includes('quickSearchRecentItem')) {
            return true;
        }

        // Quick Search Result Item
        if (className?.includes('quickSearchResultItem')) {
            return true;
        }

        // Blueprint's <FilterChip>
        if (dataset && ('radixCollectionItem' in dataset)) {
            return true;
        }

        // Blueprint's <SmallList>
        if (dataset && 'bpSmallListItem' in dataset) {
            return true;
        }

        return false;