PART 2 - configurable query suggestions & recent queries #1267
Conversation
|
Thanks for your contribution @ThibodeauJF !
|
|
Pull Request Report PR Title ❌ Title should follow the conventional commit spec: (optional scope): Example: feat(headless): add result-list controller Bundle Size
|
ThibodeauJF
requested review from
olamothe,
samisayegh and
btaillon-coveo
and removed request for
a team
ThibodeauJF
changed the title
packages/atomic/src/components/atomic-search-box/atomic-search-box.tsx
Outdated
Show resolved
Hide resolved
...-box-suggestions/atomic-search-box-query-suggestions/atomic-search-box-query-suggestions.tsx
Outdated
Show resolved
Hide resolved
...search-box-suggestions/atomic-search-box-recent-queries/atomic-search-box-recent-queries.tsx
Outdated
Show resolved
Hide resolved
| ); | ||
|
|
||
| return { | ||
| position: Array.from(this.host.parentNode!.children).indexOf(this.host), |
There was a problem hiding this comment.
is that parentNode! actually safe ? In theory, it could be possible to have someone create a search box element in JS and not attach it to the DOM, no ?
There was a problem hiding this comment.
I would expect it to be, but I'll handle it just in case (ps: this is the child query suggestion configuration element)
| private disableFeature() { | ||
| if (!this.userWarned) { | ||
| this.userWarned = true; | ||
| this.bindings.engine.logger.warn( |
There was a problem hiding this comment.
I think there would be a way to remove the userWarned flag by using a once(()=> [... the log message ...]) kinda pattern in a utility file.
| SearchBoxSuggestionsBindings, | ||
| } from '../suggestions-common'; | ||
|
|
||
| const localStorageKey = 'recent-queries'; |
There was a problem hiding this comment.
It's still early, but I think we can safely assume that we will be re-using local storage features in the future in Atomic.
If only for the need to be scoped to a specific namespace (coveo), doing safe read for iOS in incognito mode (local storage is undefined in those case) and other things you might discover of you look into what is done for JSUI, I would vote to create a utility module to interact with local storage.
This can be done in another PR, but ... I think it's important to do it sooner than later. We've had quite a lot of bugs due to local storage in JSUI.
There was a problem hiding this comment.
Okay! I'll look into it 👀
| }; | ||
| } | ||
|
|
||
| private retrieveLocalStorage() { |
There was a problem hiding this comment.
Related to above comment about local storage module, retrieve and update operations to local storage should be treated as operations that can throw.
Part 1: #1261
Final Result:
