From f27e1d505e0f8c75905365e46e1ca4e89b06edde Mon Sep 17 00:00:00 2001 From: Jason Henriquez Date: Sat, 28 Dec 2024 14:01:38 -0600 Subject: [PATCH] Implement code review changes Fix preventDefault being called for arrow left/right in search input. Revert ellipsis changes for overly long search results. Adjust matching search history logic to be non-heuristic. --- src/constants.js | 6 +++- src/renderer/components/ft-input/ft-input.css | 3 -- src/renderer/components/ft-input/ft-input.js | 32 ++++++++----------- src/renderer/components/top-nav/top-nav.js | 3 +- src/renderer/store/modules/search-history.js | 16 ++-------- src/renderer/views/Search/Search.js | 8 ++++- 6 files changed, 31 insertions(+), 37 deletions(-) diff --git a/src/constants.js b/src/constants.js index 33da8dd0587cc..53d4612bdc65d 100644 --- a/src/constants.js +++ b/src/constants.js @@ -193,9 +193,12 @@ const PLAYLIST_HEIGHT_FORCE_LIST_THRESHOLD = 500 // YouTube search character limit is 100 characters const SEARCH_CHAR_LIMIT = 100 -// matches # of results we show for search suggestions +// max # of results we show for search suggestions const SEARCH_RESULTS_DISPLAY_LIMIT = 14 +// max # of search history results we show when mixed with YT search suggestions +const MIXED_SEARCH_HISTORY_ENTRIES_DISPLAY_LIMIT = 4 + // Displayed on the about page and used in the main.js file to only allow bitcoin URLs with this wallet address to be opened const ABOUT_BITCOIN_ADDRESS = '1Lih7Ho5gnxb1CwPD4o59ss78pwo2T91eS' @@ -209,5 +212,6 @@ export { PLAYLIST_HEIGHT_FORCE_LIST_THRESHOLD, SEARCH_CHAR_LIMIT, SEARCH_RESULTS_DISPLAY_LIMIT, + MIXED_SEARCH_HISTORY_ENTRIES_DISPLAY_LIMIT, ABOUT_BITCOIN_ADDRESS, } diff --git a/src/renderer/components/ft-input/ft-input.css b/src/renderer/components/ft-input/ft-input.css index 9587079ad6665..b9904e56f6e64 100644 --- a/src/renderer/components/ft-input/ft-input.css +++ b/src/renderer/components/ft-input/ft-input.css @@ -207,9 +207,6 @@ body[dir='rtl'] .ft-input-component.search.showClearTextButton:focus-within .inp padding-block: 0; line-height: 2rem; padding-inline: 15px; - text-overflow: ellipsis; - overflow-x: hidden; - white-space: nowrap; } .searchResultIcon { diff --git a/src/renderer/components/ft-input/ft-input.js b/src/renderer/components/ft-input/ft-input.js index 2fef45a71c890..68f305ffb151e 100644 --- a/src/renderer/components/ft-input/ft-input.js +++ b/src/renderer/components/ft-input/ft-input.js @@ -295,37 +295,33 @@ export default defineComponent({ this.searchState.showOptions = true - const isArrowKeyPress = event.key.startsWith('Arrow') - - if (!isArrowKeyPress) { + // "select" the Remove button through right arrow navigation, and unselect it with the left arrow + if (event.key === 'ArrowRight') { + this.removeButtonSelectedIndex = this.searchState.selectedOption + } else if (event.key === 'ArrowLeft') { + this.removeButtonSelectedIndex = -1 + } else if (event.key === 'ArrowUp' || event.key === 'ArrowDown') { + event.preventDefault() + const newIndex = this.searchState.selectedOption + (event.key === 'ArrowDown' ? 1 : -1) + this.updateSelectedOptionIndex(newIndex) + } else { const selectedOptionValue = this.searchStateKeyboardSelectedOptionValue // Keyboard selected & is char if (!isNullOrEmpty(selectedOptionValue) && isKeyboardEventKeyPrintableChar(event.key)) { // Update input based on KB selected suggestion value instead of current input value event.preventDefault() this.handleInput(`${selectedOptionValue}${event.key}`) - return } - return - } - - event.preventDefault() - if (event.key === 'ArrowDown' || event.key === 'ArrowUp') { - const newIndex = this.searchState.selectedOption + (event.key === 'ArrowDown' ? 1 : -1) - this.updateSelectedOptionIndex(newIndex) - - // unset selection of "Remove" button - this.removeButtonSelectedIndex = -1 - } else { - // "select" the Remove button through right arrow navigation, and unselect it with the left arrow - const newIndex = event.key === 'ArrowRight' ? this.searchState.selectedOption : -1 - this.removeButtonSelectedIndex = newIndex } }, // Updates the selected dropdown option index and handles the under/over-flow behavior updateSelectedOptionIndex: function (index) { this.searchState.selectedOption = index + + // unset selection of "Remove" button + this.removeButtonSelectedIndex = -1 + // Allow deselecting suggestion if (this.searchState.selectedOption < -1) { this.searchState.selectedOption = this.visibleDataList.length - 1 diff --git a/src/renderer/components/top-nav/top-nav.js b/src/renderer/components/top-nav/top-nav.js index 6b848d551a27b..9f2255356b928 100644 --- a/src/renderer/components/top-nav/top-nav.js +++ b/src/renderer/components/top-nav/top-nav.js @@ -5,7 +5,7 @@ import FtProfileSelector from '../ft-profile-selector/ft-profile-selector.vue' import FtIconButton from '../ft-icon-button/ft-icon-button.vue' import debounce from 'lodash.debounce' -import { IpcChannels, KeyboardShortcuts, MOBILE_WIDTH_THRESHOLD, SEARCH_RESULTS_DISPLAY_LIMIT } from '../../../constants' +import { IpcChannels, KeyboardShortcuts, MIXED_SEARCH_HISTORY_ENTRIES_DISPLAY_LIMIT, MOBILE_WIDTH_THRESHOLD, SEARCH_RESULTS_DISPLAY_LIMIT } from '../../../constants' import { localizeAndAddKeyboardShortcutToActionTitle, openInternalPath } from '../../helpers/utils' import { translateWindowTitle } from '../../helpers/strings' import { clearLocalSearchSuggestionsSession, getLocalSearchSuggestions } from '../../helpers/api/local' @@ -127,6 +127,7 @@ export default defineComponent({ latestMatchingSearchHistoryNames: function () { return this.$store.getters.getLatestMatchingSearchHistoryNames(this.lastSuggestionQuery) + .slice(0, MIXED_SEARCH_HISTORY_ENTRIES_DISPLAY_LIMIT) }, latestSearchHistoryNames: function () { diff --git a/src/renderer/store/modules/search-history.js b/src/renderer/store/modules/search-history.js index a0494f4eb0592..888e63451cd01 100644 --- a/src/renderer/store/modules/search-history.js +++ b/src/renderer/store/modules/search-history.js @@ -1,9 +1,6 @@ import { SEARCH_RESULTS_DISPLAY_LIMIT } from '../../../constants' import { DBSearchHistoryHandlers } from '../../../datastores/handlers/index' -// maximum number of search history results to display when mixed with regular YT search suggestions -const MIXED_SEARCH_HISTORY_ENTRIES_DISPLAY_LIMIT = 4 - const state = { searchHistoryEntries: [] } @@ -18,18 +15,11 @@ const getters = { }, getLatestMatchingSearchHistoryNames: (state) => (name) => { - const matches = [] - for (const entry of state.searchHistoryEntries) { - if (entry.name.startsWith(name)) { - matches.push(entry.name) - if (matches.length === MIXED_SEARCH_HISTORY_ENTRIES_DISPLAY_LIMIT) { - break - } - } - } + const matches = state.searchHistoryEntries.filter((entry) => entry.name.startsWith(name)) // prioritize more concise matches - return matches.toSorted((a, b) => a.length - b.length) + return matches.map((entry) => entry.name) + .toSorted((a, b) => a.length - b.length) }, getSearchHistoryEntryWithId: (state) => (id) => { diff --git a/src/renderer/views/Search/Search.js b/src/renderer/views/Search/Search.js index 820c6eed7249f..1bdf9f34539a6 100644 --- a/src/renderer/views/Search/Search.js +++ b/src/renderer/views/Search/Search.js @@ -50,6 +50,10 @@ export default defineComponent({ showFamilyFriendlyOnly: function() { return this.$store.getters.getShowFamilyFriendlyOnly }, + + enableSearchSuggestions: function () { + return this.$store.getters.getEnableSearchSuggestions + }, }, watch: { $route () { @@ -145,7 +149,9 @@ export default defineComponent({ } } - this.updateSearchHistoryEntry() + if (this.enableSearchSuggestions) { + this.updateSearchHistoryEntry() + } }, performSearchLocal: async function (payload) {