feat(atomic): add configurable query suggestions & recent queries to search box #1261

ThibodeauJF · 2021-09-28T13:20:10Z

github-actions · 2021-09-28T13:20:31Z

Thanks for your contribution @ThibodeauJF !
When your pull-request is ready to be merged, check the box below to merge it

Merge!

github-actions · 2021-09-28T13:30:03Z

Pull Request Report

PR Title

❌ Title should follow the conventional commit spec:

(optional scope):

Example:

feat(headless): add result-list controller

Bundle Size

File	Compression	Old (kb)	New (kb)
headless.js	bundled	371.9	371.9
	minified	165.4	165.4
	gzipped	48.1	48.1
headless.esm.js	bundled	362.2	362.2
	minified	206.2	206.2
	gzipped	52.8	52.8

samisayegh · 2021-09-28T17:37:14Z

packages/atomic/src/components/atomic-search-box/atomic-search-box.tsx


  public initialize() {
    this.id = randomID('atomic-search-box-');
    this.querySetActions = loadQuerySetActions(this.bindings.engine);
    this.searchBox = buildSearchBox(this.bindings.engine, {
      options: {
        id: this.id,
-        numberOfSuggestions: 8, // TODO: handle when adding query suggestion component
+        numberOfSuggestions: 0,


Why is the new initial value 0?

We can't let the search box manage the registering and fetching of query suggestions, mainly for the promise we need to wait for before rendering all suggestions. The other atomic-search-box-query-suggestions handles it

samisayegh · 2021-09-28T17:53:52Z

...-box-suggestions/atomic-search-box-query-suggestions/atomic-search-box-query-suggestions.tsx

+  tag: 'atomic-search-box-query-suggestions',
+  shadow: true,
+})
+export class AtomicSearchBoxQuerySuggestions {


I think ending the name with a singular would make it easier to understand? Could it also be possible to shorten by removing SearchBox ? e.g.

AtomicSearchBoxQuerySuggestionList AtomicQuerySuggestionList

Fair enough, but I wanted it to be clear they are for the search box and you can't see them as independent. I could see these AtomicQuerySuggestions/AtomicRecentQueries being standalone somewhere in the page, like quantic uses recent queries

samisayegh · 2021-09-28T18:04:12Z

...-box-suggestions/atomic-search-box-query-suggestions/atomic-search-box-query-suggestions.tsx

+        ({engine, id, searchBoxController, numberOfQueries}) => {
+          const {registerQuerySuggest, fetchQuerySuggestions} =
+            loadQuerySuggestActions(engine);
+
+          (engine as SearchEngine<QuerySuggestionSection>).dispatch(
+            registerQuerySuggest({
+              id,
+              count: numberOfQueries,
+            })
+          );
+
+          return {
+            onInput: () =>
+              (engine as SearchEngine<QuerySuggestionSection>).dispatch(
+                fetchQuerySuggestions({
+                  id,
+                })
+              ),
+            renderItems: () =>
+              // TODO: limit values according to maxWithQuery/maxWithoutQuery
+              searchBoxController.state.suggestions.map((suggestion) => ({
+                content: <span innerHTML={suggestion.highlightedValue}></span>,
+                value: suggestion.rawValue,
+                onClick: () =>
+                  searchBoxController.selectSuggestion(suggestion.rawValue),


I think this block would be easier to follow if it was broken up into multiple smaller functions.

Yep! I'll do it in the other PR

samisayegh · 2021-09-28T18:08:25Z

packages/atomic/src/components/search-box-suggestions/suggestions-common.ts

+  );
+  if (canceled) {
+    throw new Error(
+      'The Atomic search box suggestion component was not handled, as it is not a child of a search box component'


I think it would be nice to include the tag, or an example, in the message to help devs understand, have something that they can copy-paste in case of a typo. e.g.

<atomic-search-box> <atomic-search-box-suggestions/> </atomic-search-box>

samisayegh · 2021-09-28T18:09:45Z

packages/atomic/src/components/search-box-suggestions/suggestions-common.ts

+  element: Element
+) => {
+  const canceled = element.dispatchEvent(
+    buildCustomEvent('atomic/searchBoxSuggestion', event)


Would the event's role be clearer if it included /register or similar at the end? e.g.

atomic/searchBoxSuggestion/register

olamothe · 2021-09-28T18:32:58Z

...-box-suggestions/atomic-search-box-query-suggestions/atomic-search-box-query-suggestions.tsx

+
+  @State() public error!: Error;
+
+  @Prop() public maxWithQuery?: number;


Do we really need two different prop for this ? Just one for both could do, no ? 🤔

Why would it be important to have separate option for both mode ?

From the design (figma), you put as many recent queries as possible when the input is empty, but then you match up to 3 queries. I could remove the options here for query suggest, but I find it more consistent.

olamothe · 2021-09-28T18:45:56Z

packages/atomic/src/components/atomic-search-box/atomic-search-box.tsx

+   *  - atomic-search-box-query-suggestions
+   *  - atomic-search-box-recent-queries
+   */
+  @Prop() public numberOfQueries = 8;


numberOfSuggestions ? What do you think ?

Like described in the documents, there will also be "numberOfResults" for the other types of suggestions. So these scenarios can be configured:

I think that instead of making this an option here, we could have an opiniated default, and if people want to override it then they need to specify atomic-search-box-query-suggestions and atomic-search-box-recent-queries themselves.

Otherwise, I find it confusing to predict what number of suggestions & recent queries will each appear based on this option.

This is why each of the sub elements, atomic-search-box-query-suggestions and atomic-search-box-recent-queries, have maximums, which when not defined, fill up the numberOfQueries value. How else could we configure the following:

I want a max of 8 queries at all times (any type)

When there is no query in the input, I want all the recent queries I can up to 8, but fill the rest with query suggestions

When there is a query, I want up to 3 matching recent queries, and fill the rest with query suggestions

olamothe · 2021-09-28T19:12:26Z

packages/atomic/src/components/search-box-suggestions/suggestions-common.ts

+import {buildCustomEvent} from '../../utils/event-utils';
+import {Bindings} from '../../utils/initialization-utils';
+
+export interface SearchBoxSuggestionElement {


When we want to make the "Custom Atomic component" API official, it means we will need to make the mechanism to extend "search box suggestions" provider public. And that can be a pretty tricky thing to get right.

A couple notes/thoughts:

Since the content (the VNode) is ultimately rendered inside an ul / li in atomic-search-box, I guess that not all HTML elements would be valid ? I'm perfectly fine with this, by the way, but if there's any restriction, they need to be clear and (potentially) automatically handled (read: rejected) by the system so that users don't get random errors. Another approach would be to accept anything here, but to make it possible to control the "container" in atomic-search-box (being able to change ul / li to something else. This does add complexity, however.

If someone wants to provide very rich/complex suggestion (eg: A product image with lots of metadata section), how would the keyboard navigation plays into that ? If you could navigate to different subsection on a suggestion. Should the system forbid that ?

What happens if you want to have multiple clickable/interactive element in a given suggestion element ? Would the onClick cause a problem then ?

Is the integration of that dispatchSearchBoxSuggestionsEvent inside the componentWillLoad function as clean as it could be ? Could it be a more straightforward interface that you need to implement (or clever usage of an annotation).

I want that limitation, the search box manages all the interactivity with the search box. The li is needed to be correctly accessible too.

Yes, a single element means a single possible interaction, more than that get's too complex for all our use cases.

Same as point 2

What would you propose like interface? I will certainly be extended upon in the future, modify with our needs. It's not public yet and before it is we will have improved upon it to support instant results and recently viewed.

olamothe · 2021-09-28T19:13:50Z

packages/atomic/src/components/search-box-suggestions/suggestions-common.ts

+import {Bindings} from '../../utils/initialization-utils';
+
+export interface SearchBoxSuggestionElement {
+  value: string;


Unless I am mistaken, we use value here as a key for JSX code.

Could we just name it key then ?

This could change when adding results, but the way suggestion worked is the value (used for key & data-value) will affect the query when you use the arrows, just like on google.com & the JSUI

olamothe · 2021-09-28T19:14:36Z

...search-box-suggestions/atomic-search-box-recent-queries/atomic-search-box-recent-queries.tsx

+
+  @State() public error!: Error;
+
+  @Prop() public maxWithQuery?: number;


Similar as above, why do we need two prop for this ?

https://github.com/coveo/ui-kit/pull/1261/files#r717899897

olamothe · 2021-09-28T19:16:34Z

packages/atomic/src/components/search-box-suggestions/suggestions-common.ts

+
+export interface SearchBoxSuggestionElement {
+  value: string;
+  onClick(): void;


Suggestion: onSelect since it's not always going to be a click. eg: touch on mobile, or enter is using a keyboard.

btaillon-coveo · 2021-09-29T15:10:50Z

packages/atomic/src/components/search-box-suggestions/suggestions-common.ts

+export interface SearchBoxSuggestionElement {
+  value: string;
+  onClick(): void;
+  content: VNode;


The type VNode works great in Stencil and maybe in other JSX frameworks, but I don't know if it's possible without those frameworks to provide a VNode.

btaillon-coveo · 2021-09-29T17:12:14Z

...search-box-suggestions/atomic-search-box-recent-queries/atomic-search-box-recent-queries.tsx

+        });
+
+        return {
+          onInput: () => {},


Should we show all recent queries in the suggestions or only those that include what we typed so far?

@johnnyCoveo

It's in the mockups, when no query, we display them as is, when there is a query, we filter them out, you can refer to the part 2 for the implementation!

https://coveord.atlassian.net/browse/KIT-853

ThibodeauJF added 5 commits September 28, 2021 07:15

search box: don't register QS when numberOfSuggestions = 0

f333932

https://coveord.atlassian.net/browse/KIT-853

setup a mix of recent queries and query suggest

903ddb7

https://coveord.atlassian.net/browse/KIT-853

add children configuration elements

acd6b24

https://coveord.atlassian.net/browse/KIT-853

add default components

e30b981

https://coveord.atlassian.net/browse/KIT-853

add todos and extract custom event dispatch

87eddcf

https://coveord.atlassian.net/browse/KIT-853

ThibodeauJF requested a review from a team as a code owner September 28, 2021 13:20

ThibodeauJF requested review from olamothe, samisayegh and btaillon-coveo September 28, 2021 13:20

samisayegh approved these changes Sep 28, 2021

View reviewed changes

olamothe approved these changes Sep 28, 2021

View reviewed changes

ThibodeauJF mentioned this pull request Sep 28, 2021

PART 2 - configurable query suggestions & recent queries #1267

Merged

btaillon-coveo reviewed Sep 29, 2021

View reviewed changes

ThibodeauJF changed the title ~~feat(atomic): add configurable query suggestions & recent queries to search box (TODOs still remain, more PRs coming)~~ feat(atomic): add configurable query suggestions & recent queries to search box Sep 29, 2021

btaillon-coveo reviewed Sep 29, 2021

View reviewed changes

btaillon-coveo approved these changes Sep 29, 2021

View reviewed changes

PART 2 - configurable query suggestions & recent queries (#1267)

e5a4cdc

https://coveord.atlassian.net/browse/KIT-853

ThibodeauJF merged commit bb38b27 into master Oct 6, 2021

ThibodeauJF deleted the KIT-853 branch October 6, 2021 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(atomic): add configurable query suggestions & recent queries to search box #1261

feat(atomic): add configurable query suggestions & recent queries to search box #1261

ThibodeauJF commented Sep 28, 2021

github-actions bot commented Sep 28, 2021

github-actions bot commented Sep 28, 2021

samisayegh Sep 28, 2021

ThibodeauJF Sep 28, 2021

samisayegh Sep 28, 2021

ThibodeauJF Sep 28, 2021

samisayegh Sep 28, 2021

ThibodeauJF Sep 28, 2021

samisayegh Sep 28, 2021

samisayegh Sep 28, 2021

olamothe Sep 28, 2021

ThibodeauJF Sep 28, 2021

olamothe Sep 28, 2021

ThibodeauJF Sep 28, 2021

btaillon-coveo Sep 29, 2021 •

edited

Loading

ThibodeauJF Sep 29, 2021

olamothe Sep 28, 2021

ThibodeauJF Sep 28, 2021 •

edited

Loading

olamothe Sep 28, 2021

ThibodeauJF Sep 28, 2021

olamothe Sep 28, 2021

ThibodeauJF Sep 28, 2021

olamothe Sep 28, 2021

btaillon-coveo Sep 29, 2021

btaillon-coveo Sep 29, 2021

ThibodeauJF Sep 29, 2021


		@State() public error!: Error;

		@Prop() public maxWithQuery?: number;

feat(atomic): add configurable query suggestions & recent queries to search box #1261

feat(atomic): add configurable query suggestions & recent queries to search box #1261

Conversation

ThibodeauJF commented Sep 28, 2021

github-actions bot commented Sep 28, 2021

github-actions bot commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btaillon-coveo Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThibodeauJF Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btaillon-coveo Sep 29, 2021 •

edited

Loading

ThibodeauJF Sep 28, 2021 •

edited

Loading