fix(svelte): respect default pattern type in toggles#63795
Conversation
| // When a toggle is unset, we revert back to the default pattern type. However, if the default pattern type | ||
| // is regexp, we should revert to keyword instead (otherwise it's not possible to disable the toggle). | ||
| function getUnselectedPatternType(): SearchPatternType { | ||
| const defaultPatternType = ($settings?.['search.defaultPatternType'] as SearchPatternType) ?? SearchPatternType.keyword |
There was a problem hiding this comment.
This is my first SvelteKit PR, so not really sure what I'm doing :) And also this felt too easy.
There was a problem hiding this comment.
I'm glad you found it easy 😃
It doesn't make a big difference here since this is not a hot path, but instead of defining and calling a function every time the toggle is clicked, you could instead define a reactive/derived variable. In this case the variable would only update when $settings changes. For example:
$: unselectedPatternType = getUnselectedPatternType(
$settings?.['search.defaultPatternType'] as (SearchPatternType|undefined)
)
function getUnselectedPatternType(defaultPatternType = SearchPatternType.keyword): SearchPatternType {
return defaultPatternType === SearchPatternType.regexp
? SearchPatternType.keyword
: defaultPatternType
}
function setOrUnsetPatternType(patternType: SearchPatternType): void {
queryState.setPatternType(currentPatternType =>
currentPatternType === patternType
? unselectedPatternType
: patternType
)
}But again, that's not necessary here IMO, and the function solution might actually be easier to understand.
There was a problem hiding this comment.
Good to know about derived variables! I agree that the current approach is most straightforward though and shouldn't hurt performance.
| // When a toggle is unset, we revert back to the default pattern type. However, if the default pattern type | ||
| // is regexp, we should revert to keyword instead (otherwise it's not possible to disable the toggle). | ||
| function getUnselectedPatternType(): SearchPatternType { | ||
| const defaultPatternType = ($settings?.['search.defaultPatternType'] as SearchPatternType) ?? SearchPatternType.keyword |
There was a problem hiding this comment.
I'm glad you found it easy 😃
It doesn't make a big difference here since this is not a hot path, but instead of defining and calling a function every time the toggle is clicked, you could instead define a reactive/derived variable. In this case the variable would only update when $settings changes. For example:
$: unselectedPatternType = getUnselectedPatternType(
$settings?.['search.defaultPatternType'] as (SearchPatternType|undefined)
)
function getUnselectedPatternType(defaultPatternType = SearchPatternType.keyword): SearchPatternType {
return defaultPatternType === SearchPatternType.regexp
? SearchPatternType.keyword
: defaultPatternType
}
function setOrUnsetPatternType(patternType: SearchPatternType): void {
queryState.setPatternType(currentPatternType =>
currentPatternType === patternType
? unselectedPatternType
: patternType
)
}But again, that's not necessary here IMO, and the function solution might actually be easier to understand.
I gave this a try and added an integration test. It highlighted a difference between the React and Sveltekit versions: in the experimental app, just hitting the toggle has no effect on the URL (and does not resubmit the search). |
In Sveltekit, we respect the setting
search.defaultPatternTypefor the initial search, but if you set then unset the regexp toggle, it always reverts tokeyword. Now we revert back to the default when a toggle is unset.This is important because some customers have disabled keyword search, and for 5.5 we've directed them to use
search.defaultPatternType: standard.Test plan
Manually tested search + toggles with following config:
search.defaultPatternType: standard,keyword, andregexp