-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Make sure the first result in most recents is highlighted when user uses CMD+K #52298
Changes from 6 commits
de6e681
ac997b5
1007716
f03e1e0
dd17a40
fe78ba6
b86b260
2312a87
fd76ed7
e502f95
d3778db
913a059
e9bea23
a9ab802
7861d45
e4e2ab1
ef484ce
4457266
ee1acd6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -17,6 +17,7 @@ import usePolicy from '@hooks/usePolicy'; | |||||||||||||||||||||||||
import useResponsiveLayout from '@hooks/useResponsiveLayout'; | ||||||||||||||||||||||||||
import useThemeStyles from '@hooks/useThemeStyles'; | ||||||||||||||||||||||||||
import * as CardUtils from '@libs/CardUtils'; | ||||||||||||||||||||||||||
import getPlatform from '@libs/getPlatform'; | ||||||||||||||||||||||||||
import * as OptionsListUtils from '@libs/OptionsListUtils'; | ||||||||||||||||||||||||||
import {getAllTaxRates} from '@libs/PolicyUtils'; | ||||||||||||||||||||||||||
import type {OptionData} from '@libs/ReportUtils'; | ||||||||||||||||||||||||||
|
@@ -57,6 +58,9 @@ function SearchRouter({onRouterClose}: SearchRouterProps) { | |||||||||||||||||||||||||
const [autocompleteSuggestions, setAutocompleteSuggestions] = useState<AutocompleteItemData[] | undefined>([]); | ||||||||||||||||||||||||||
const [autocompleteSubstitutions, setAutocompleteSubstitutions] = useState<SubstitutionMap>({}); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const isWeb = getPlatform() === CONST.PLATFORM.WEB; | ||||||||||||||||||||||||||
const isDesktop = getPlatform() === CONST.PLATFORM.DESKTOP; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const {shouldUseNarrowLayout} = useResponsiveLayout(); | ||||||||||||||||||||||||||
const listRef = useRef<SelectionListHandle>(null); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
|
@@ -325,6 +329,18 @@ function SearchRouter({onRouterClose}: SearchRouterProps) { | |||||||||||||||||||||||||
Report.searchInServer(debouncedInputValue.trim()); | ||||||||||||||||||||||||||
}, [debouncedInputValue]); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const setInitialFocus = useCallback(() => { | ||||||||||||||||||||||||||
const initialFocusIndex = (sortedRecentSearches?.slice(0, 5).length ?? 0) + (contextualReportData ? 1 : 0); | ||||||||||||||||||||||||||
listRef.current?.setFocusedIndex(initialFocusIndex, false); | ||||||||||||||||||||||||||
}, [sortedRecentSearches, contextualReportData]); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
useEffect(() => { | ||||||||||||||||||||||||||
if ((!isWeb && !isDesktop) || textInputValue) { | ||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
setInitialFocus(); | ||||||||||||||||||||||||||
}, [setInitialFocus, textInputValue, isWeb, isDesktop]); | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Let search change focus be handled by onSearchChange function instead of making call to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah you are right, it calls the But this is intentional actually, if we dont early return when the See this videoNew-Expensify.35.mp4I think we should remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As of now we need to focus only for the first time we can remove this entire function and use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we use New-Expensify.mp4There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For that, we can use isLargeScreenWidth and set undefined initiallyFocusedOptionKey based on it. |
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const onSearchChange = useCallback( | ||||||||||||||||||||||||||
(userQuery: string) => { | ||||||||||||||||||||||||||
let newUserQuery = userQuery; | ||||||||||||||||||||||||||
|
@@ -340,11 +356,13 @@ function SearchRouter({onRouterClose}: SearchRouterProps) { | |||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (newUserQuery) { | ||||||||||||||||||||||||||
listRef.current?.updateAndScrollToFocusedIndex(0); | ||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||
} else if (!isWeb && !isDesktop) { | ||||||||||||||||||||||||||
listRef.current?.updateAndScrollToFocusedIndex(-1); | ||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||
setInitialFocus(); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||
[autocompleteSubstitutions, autocompleteSuggestions, setTextInputValue, updateAutocomplete], | ||||||||||||||||||||||||||
[autocompleteSubstitutions, autocompleteSuggestions, setTextInputValue, updateAutocomplete, setInitialFocus, isWeb, isDesktop], | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const onSearchSubmit = useCallback( | ||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -114,6 +114,7 @@ function BaseSelectionList<TItem extends ListItem>( | |||||||||
shouldKeepFocusedItemAtTopOfViewableArea = false, | ||||||||||
shouldDebounceScrolling = false, | ||||||||||
shouldPreventActiveCellVirtualization = false, | ||||||||||
shouldScrollToFocusedIndexOnFirstRender = true, | ||||||||||
}: BaseSelectionListProps<TItem>, | ||||||||||
ref: ForwardedRef<SelectionListHandle>, | ||||||||||
) { | ||||||||||
|
@@ -316,12 +317,14 @@ function BaseSelectionList<TItem extends ListItem>( | |||||||||
maxIndex: Math.min(flattenedSections.allOptions.length - 1, CONST.MAX_SELECTION_LIST_PAGE_LENGTH * currentPage - 1), | ||||||||||
disabledIndexes: disabledArrowKeyIndexes, | ||||||||||
isActive: isFocused, | ||||||||||
onFocusedIndexChange: (index: number) => { | ||||||||||
onFocusedIndexChange: (index: number, shouldScrollToIndex = true) => { | ||||||||||
const focusedItem = flattenedSections.allOptions.at(index); | ||||||||||
if (focusedItem) { | ||||||||||
onArrowFocus(focusedItem); | ||||||||||
} | ||||||||||
(shouldDebounceScrolling ? debouncedScrollToIndex : scrollToIndex)(index, true); | ||||||||||
if (shouldScrollToIndex) { | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think we don't need any changes in shouldScrollToFocusedIndex={!isInitialRender} |
||||||||||
(shouldDebounceScrolling ? debouncedScrollToIndex : scrollToIndex)(index, true); | ||||||||||
} | ||||||||||
}, | ||||||||||
isFocused, | ||||||||||
}); | ||||||||||
|
@@ -586,10 +589,12 @@ function BaseSelectionList<TItem extends ListItem>( | |||||||||
if (!isInitialSectionListRender) { | ||||||||||
return; | ||||||||||
} | ||||||||||
scrollToIndex(focusedIndex, false); | ||||||||||
if (shouldScrollToFocusedIndexOnFirstRender) { | ||||||||||
scrollToIndex(focusedIndex, false); | ||||||||||
} | ||||||||||
setIsInitialSectionListRender(false); | ||||||||||
}, | ||||||||||
[focusedIndex, isInitialSectionListRender, scrollToIndex, shouldUseDynamicMaxToRenderPerBatch], | ||||||||||
[focusedIndex, isInitialSectionListRender, scrollToIndex, shouldUseDynamicMaxToRenderPerBatch, shouldScrollToFocusedIndexOnFirstRender], | ||||||||||
); | ||||||||||
|
||||||||||
const onSectionListLayout = useCallback( | ||||||||||
|
@@ -713,12 +718,13 @@ function BaseSelectionList<TItem extends ListItem>( | |||||||||
isTextInputFocusedRef.current = isTextInputFocused; | ||||||||||
}, []); | ||||||||||
|
||||||||||
useImperativeHandle(ref, () => ({scrollAndHighlightItem, clearInputAfterSelect, updateAndScrollToFocusedIndex, updateExternalTextInputFocus, scrollToIndex}), [ | ||||||||||
useImperativeHandle(ref, () => ({scrollAndHighlightItem, clearInputAfterSelect, updateAndScrollToFocusedIndex, updateExternalTextInputFocus, scrollToIndex, setFocusedIndex}), [ | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
scrollAndHighlightItem, | ||||||||||
clearInputAfterSelect, | ||||||||||
updateAndScrollToFocusedIndex, | ||||||||||
updateExternalTextInputFocus, | ||||||||||
scrollToIndex, | ||||||||||
setFocusedIndex, | ||||||||||
]); | ||||||||||
|
||||||||||
/** Selects row when pressing Enter */ | ||||||||||
|
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.
This goes against our best practices - https://github.com/Expensify/App/blob/0dd44ee996d7d2b41ec117205fb52b10a33cf5f0/README.md#platform-specific-file-extensions
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.
@Pujan92 should we create platform-specific files for this case?