Adapt predictive search layout to new api #2192

FCalabria · 2023-01-04T14:55:18Z

PR Summary:

Update the predictive search component to take advantage of the new default values in the API and support all resource types

⚠️ Feature requires a flag on the shop to actually return query suggestions. This won't be available publicly for some time; and public docs will be updated once this is available.

Why are these changes introduced?

https://github.com/Shopify/discovery/issues/6681

New default values on resource[types] https://github.com/Shopify/discovery/issues/6617

What approach did you take?

The two column structure has been done with flex, using the flex-direction property to change between the mobile and desktop structure, with the appropiate changes in flex-grow for the children to fill the space when needed.

The trickiest part is the "Pages" section, that is placed in a different column depending on the screen size. I solved this duplicating the html, adding some conditional classes to the wrapper and using them with media queries to hide the page section that was not needed.

Very simplified, the final html structure looks like this

<div id="predictive-search-results">
  <div class="predictive-search__results-groups-wrapper"> <!-- This div gets conditional .predictive-search__results-groups-wrapper--no-products and .predictive-search__results-groups-wrapper--no-suggestions -->
    <div class="predictive-search__result-group"> <!-- This div is not present if it has no content (it could be present but with hidden pages on mobile) -->
      <div> <!-- Not present if it has no content -->
        <h2 id="predictive-search-queries" class="predictive-search__heading">
          Suggestions
        </h2>
        <ul
          id="predictive-search-results-queries-list"
          class="predictive-search__results-list"
        >
          <li class="predictive-search__list-item" aria-selected="false">
            <a class="predictive-search__item"> </a>
          </li>
        </ul>
      </div>
      <div class="predictive-search__pages-wrapper"> <!-- Not present if it has no content (it could be present but with hidden pages on mobile)-->
        <h2 id="predictive-search-pages" class="predictive-search__heading">
          Pages
        </h2>
        <!-- Similar ul > li, changing the ids -->
      </div>
    </div>
    <div class="predictive-search__result-group"> <!-- This div is not present if it has no content (it could be present but with hidden pages on desktop) -->
      <div> <!-- Not present if it has no content -->
        <h2 id="predictive-search-products" class="predictive-search__heading">
          Products
        </h2>
        <!-- Similar ul > li, changing the ids -->
      </div>
      <div class="predictive-search__pages-wrapper"> <!-- Not present if it has no content (it could be present but with hidden pages on desktop)-->
        <h2 id="predictive-search-pages" class="predictive-search__heading">
          Pages
        </h2>
        <!-- Similar ul > li, changing the ids -->
      </div>
    </div>
  </div>
  <div class="predictive-search__loading-state" aria-hidden="true"></div> <!-- Loading spinner -->

  <div id="predictive-search-option-search-keywords">
    <button class="predictive-search__item predictive-search__item--term">
      Search for “c”
    </button>
  </div>
</div>

Visual impact on existing themes

Users will start to get more type of results on the predictive search. They will also see the max number of results increased to 10

Testing steps/scenarios

Figma for reference

The predictive search dropdown can show all possible resource types (even the ones that are not queried by default)
The predictive search dropdown looks as expected even when some of the resource types are not populated
The predictive search dropdown is still accessible

Demo links

Checklist

Added PR summary for release notes
Requested review from UX (Only for changes that are affecting the experience or perceivable visual details)
Created a ticket for the help.shopify.com documentation team about updates to theme settings. (Internal-only task)
Followed theme code principles
Linted with Theme Check
Tested on mobile
Tested on multiple browsers
Tested for accessibility

Support all resource types

sections/predictive-search.liquid

NathanPJF

Requesting changes for the aria-hidden text.

FYI: I made a codepen where I used grid-template-area to manage the display in order to avoid duplicating the HTML. However, I realized it would make the JS a lot more complicated because the order of the DOM nodes couldn't be relied upon to match the visual order. Similar to focus order issues with Grid talked about here on MDN. So ultimately I think the repeated HTML is fine here.

sections/predictive-search.liquid

assets/predictive-search.js

sections/predictive-search.liquid

melissaperreault · 2023-01-05T21:15:10Z

assets/component-predictive-search.css

+  display: flex;
+  flex-direction: column;
+  flex: 1 1 auto;
+  gap: 3rem;


Suggested change

gap: 3rem;

gap: 2rem;

I find the 3rem gap to be very large. Suggesting 2rem to balance white space. Thoughts @al-mcquiston ? (Visual reference at 2rem for desktop, this is on Mobile)

@melissaperreault totally agree. myself and @FCalabria chatted about this and this should be updated to your suggestion

melissaperreault

Thank you Paqui! I took a quick look and will continue tomorrow! Only feedback would be about not being convinced we need 3 times the loading spinner as the query is typed. @al-mcquiston

NathanPJF

Nit about the use of h3 for the list options. These can be safely changed to p tags as the .h5 class overwrites all the styling.

Tested on VoiceOver in Mobile Safari, Safari, and Chrome. And JAWS in Edge. The arrow keys navigation isn't picked up in Safari unfortunately, but I can walkthrough the page correctly with a virtual cursor (hence the addition of an aria-label on the queries list)

sections/predictive-search.liquid

NathanPJF · 2023-01-05T22:35:09Z

sections/predictive-search.liquid

+                        <h3 class="predictive-search__item-heading h5">{{ product.title }}
+                        </h3>


Suggested change

<h3 class="predictive-search__item-heading h5">{{ product.title }}

</h3>

<p class="predictive-search__item-heading h5">{{ product.title }}</p>

I wouldn't say I disagree with this suggestion, but just FYI it looks like during a11y review of the original implementation of predictive search it was specifically recommended that these product titles use h3. Though I could maybe see that being more relevant if the vendor or price is also showing. If we're confident about this change though, feel free.

Thanks for pointing that out. I read the past PR; the comment about using an h2 here made sense to me; but I don't understand why an h3 was recommended. These node don't have any child content - so I don't see it as a "heading" for any section. @svinkle : do you recall why you recommended an h3 in this past commit?

Given we're using listbox and identifying group roles, I don't believe we need this h3 to communicate structure. https://www.w3.org/WAI/ARIA/apg/example-index/listbox/listbox-grouped.html. However, I don't think h3 was communicating structure in the last PR either because the heading was put on the lowest text node in the list 🤔

do you recall why you recommended an h3 in this past commit?

I do not. As mentioned, the heading structure is not conveyed here. Swap for a generic container.

sections/predictive-search.liquid

NathanPJF · 2023-01-05T22:57:07Z

@melissaperreault you wrote

not being convinced we need 3 times the loading spinner as the query is typed

I had noticed the pattern for loading is a bit strange as well. Though something to note is that there isn't always 3 loaders. The number of loaders depends on how many resource headings (suggestions, pages, products) were on the page before the next search.

📹 In this video, I slow down the network connection, and the following can be seen:

I type "L" and there is a loader just in the middle of the page. I get results for both suggestions and products
I type "Z" and now there are 2 loaders... but I only get products back
I type "V", I only get 1 loader, but then 3 resource types back (suggestions, pages, products)

The loading indication could be changed. This approach may have made sense when 'Products' was the only possible heading you'd see, but now the number of resource types you get back is dynamic. Perhaps the loaders next to the resources headings should be removed completely - so no indication that anything is happening, the next content will just pop into existence.

Related, but separate issue to this PR: there's a larger issue with how loading is already being done IMO. The contents of the listbox aren't cleared between searches. This makes sense for as you're typing, but not so much when the search box is emptied; as a result, you will briefly see the results of the previous search when you start a new search. Video example where I show typing "V", clearing results, then type "T". I see the results for "V" as my new results are being loaded. This issue exists in the search box that's in production.

bakura10 · 2023-01-06T01:57:41Z

Maestrooo developers here. Just found this. Do you have any documentation? As this feature may have visual implication on our themes I would like to start working on this as soon as possible. I have tried to add a new "query" resource type but I have a 422 error.

As always, we would appreciate if we (theme dev) could be also added in the loop for important upgrades like this, it would also eventually allow us to provide feedback.

translation-platform · 2023-01-09T12:35:37Z

We have received a translation request but there is a question blocking translation work. Your help is needed.

Please answer the following questions.

Warning
Replies in GitHub are not visible to translators. Use the provided "Answer here" links. 🙅‍♀️

The count tag says "ONE", so "suggestions" should be singular.I will translate following the tag for now. Answer here.

dawn/locales/en.default.json

Line 210 in e495b48

"one": "{{ count }} suggestions",

Note
Not your issue? Forward it to someone with more context; please don't leave it pending. 🙏
Questions? Hop in the #help-i18n-and-translation Slack channel.

FCalabria · 2023-01-09T17:59:04Z

Related, but separate issue to this PR: there's a larger issue with how loading is already being done IMO. The contents of the listbox aren't cleared between searches. This makes sense for as you're typing, but not so much when the search box is emptied; as a result, you will briefly see the results of the previous search when you start a new search. Video example where I show typing "V", clearing results, then type "T". I see the results for "V" as my new results are being loaded. This issue exists in the search box that's in production.

@NathanPJF I have created this issue to fix this problem later, since it is a current bug and not caused by my changes. Take a look in case you want to suggest any changes.

About the loading, I talked with @al-mcquiston and we decided to remove the loading icons on the headers, since that seem to be a common pattern in other websites.

kmeleta

I'm still doing some testing (a11y in particular) but overall I think this is looking good. I'm raising a couple UX questions and some code formatting stuff, but I haven't come across anything too alarming or blocking yet.

kmeleta · 2023-01-09T16:26:22Z

assets/component-predictive-search.css

 .predictive-search__heading {
-  border-bottom: 0.1rem solid rgba(var(--color-foreground), 0.08);
+  border-bottom: 0.1rem solid rgba(var(--color-foreground), 0.2);


It seems like for this use case we typically use 0.08, though I do see a couple instances of 0.2 in the cart. Will let @melissaperreault's weigh in though if this the added opacity is warranted. (note there's an additional instance of this value above around line 27.

Not necessary to add here, but Dawn should probably already have this sort of value as a global var to ensure consistency..

+1 The opacity should not change here and remain at 0.08.

Another point I just noticed; I am not sure what happened but we lost the type specific dividers here. When I initially reviewed, they were in place properly, now they just blend from left to right. The initial layout is best to maintain hierarchy between the results and the CTA at the bottom of the box.

Looks like the borders are extending to the left of the headings now. I don't believe I've see any discussion about this here or in the figma so I assume it's unintentional.

@melissaperreault "we lost the type specific dividers here" I had just noticed this too! Definitely want to get this reverted back.

Updating opacity to 0.08 sounds good to me!

Regression fixed in latest commit

sections/header.liquid

sections/predictive-search.liquid

assets/predictive-search.js

carolinjahn · 2023-01-09T23:14:34Z

@bakura10 the API changes are not yet ready for consumption. Theme developer comms should be going out soon (1-2 weeks) with more information ahead of the launch. If you have any specific feedback, feel free to DM me on partner slack.

melissaperreault · 2023-01-10T21:40:27Z

Adding last piece of observations!

Transition

*This feedback is not launch blocking but something I'd like us to improve.

Perhaps the loaders next to the resources headings should be removed completely - so no indication that anything is happening, the next content will just pop into existence.

Yeah, this is the first impression that I would hope we could spend more time in a follow up task to improve the perceived performance. I am going to throw a few buzz word with hope that it helps convey in which direction we can explore: it could be smoother, snappier; buyers don't need to hold on their toes to only find out there is none of X result type. I am not sure what is the common pattern but I think we can explore further.

Would a fade in animation instead help? Is 3 relevant because things are very likely to load at different times? Could customers have the perception that things all load together, all load separate, populates as it loads (e.g. skeleton) . I just wonder if we can build an opinion of what is the most ideal experience for the buyers.

About the loading, I talked with @al-mcquiston and we decided to remove the loading icons on the headers, since that seem to be a common pattern in other websites

We need to keep at least 1 spinner though. This will help the customer waiting as they type their query on slower connection to not be holding their breath with an empty screen.

kmeleta · 2023-01-10T21:31:16Z

assets/component-predictive-search.css

 .predictive-search__heading {
-  border-bottom: 0.1rem solid rgba(var(--color-foreground), 0.08);
+  border-bottom: 0.1rem solid rgba(var(--color-foreground), 0.2);


Looks like the borders are extending to the left of the headings now. I don't believe I've see any discussion about this here or in the figma so I assume it's unintentional.

kmeleta · 2023-01-10T21:44:23Z

sections/main-search.liquid

@@ -20,7 +20,7 @@

  .template-search__search {


Group headings are centered on the search page.

Fixed by moving the center class off the container to the heading.

NathanPJF · 2023-01-11T05:44:47Z

sections/predictive-search.liquid

@@ -15,7 +15,7 @@
      {%- if predictive_search.resources.queries.size > 0 or predictive_search.resources.collections.size > 0 -%}
        <div>
          <h2 id="predictive-search-queries" class="predictive-search__heading text-body caption-with-letter-spacing">
-            {{ 'templates.search.suggestions' | t }}
+            {{- 'templates.search.suggestions' | t -}}


While reviewing the output, I noticed that the whitespaces were being rendered in the inspector. This doesn't have an impact on the UI really, but it makes the output more inline with what we want.

NathanPJF

My comments about the output in VoiceOver and JAWS have been addressed, and the feature works as expected. Nice work @FCalabria !

melissaperreault

Thank you! Looks good to me!

Tiny detail for when we tackle loading states, we need to consider this use case too for when no results are found and are only left with the CTA. (video example)

kmeleta

Approved. Overall, the feature works well and I see no real issues with the implementation 👍 Nice work!

I do find the Voiceover experience in safari a little unfortunate in that arrow key navigation doesn't provide much [if any] feedback, nor does it announce the results live region updates. However, the markup here looks to spec (announcements in JAWS work well) and these VO issues seem to be preexisting in dawn, so I can't really consider these issues to be blocking.

Given the increased complexity in the search results with this change though, I do think it would be nice if you could timebox a follow-up to see if we can make any improvements here at all. I believe there is also some ongoing UX discussion around loading states and we would appreciate any effort to come to a solution in a follow-up as well. Let me know if we can help with any of this.

Adapt predictive search layout to new api Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> Co-authored-by: Nathan Ferguson <nathan.pj.ferguson@gmail.com>

Adapt predictive search layout to new api

79b785c

Support all resource types

FCalabria requested a review from al-mcquiston January 4, 2023 15:14

melissaperreault self-requested a review January 4, 2023 15:31

nicklepine requested a review from LucasLacerdaUX January 4, 2023 17:38

NathanPJF requested changes Jan 4, 2023

View reviewed changes

sections/predictive-search.liquid Outdated Show resolved Hide resolved

sections/predictive-search.liquid Outdated Show resolved Hide resolved

sections/predictive-search.liquid Outdated Show resolved Hide resolved

sections/predictive-search.liquid Outdated Show resolved Hide resolved

PR fixes

2a03fb9

FCalabria requested a review from NathanPJF January 4, 2023 19:48

NathanPJF requested changes Jan 4, 2023

View reviewed changes

sections/predictive-search.liquid Outdated Show resolved Hide resolved

assets/predictive-search.js Show resolved Hide resolved

assets/predictive-search.js Outdated Show resolved Hide resolved

NathanPJF reviewed Jan 4, 2023

View reviewed changes

assets/predictive-search.js Outdated Show resolved Hide resolved

FCalabria requested a review from kmeleta January 5, 2023 15:00

Update 21 translation files

009b095

NathanPJF requested changes Jan 5, 2023

View reviewed changes

sections/predictive-search.liquid Outdated Show resolved Hide resolved

sections/predictive-search.liquid Show resolved Hide resolved

A11y fixes

4a5f90d

FCalabria force-pushed the new-predictive-search-api-adaptation branch from 41cbcca to 4a5f90d Compare January 5, 2023 17:30

FCalabria requested a review from NathanPJF January 5, 2023 17:33

Update 12 translation files

45974a6

melissaperreault reviewed Jan 5, 2023

View reviewed changes

NathanPJF previously approved these changes Jan 5, 2023

View reviewed changes

Update 8 translation files

b71439b

translation-platform bot dismissed NathanPJF’s stale review via b71439b January 6, 2023 17:04

Update 11 translation files

e495b48

FCalabria added 2 commits January 9, 2023 16:27

Label fix

0e154d3

PR small fixes

42c8a6c

kmeleta reviewed Jan 9, 2023

View reviewed changes

FCalabria force-pushed the new-predictive-search-api-adaptation branch from 4e7922a to 80b0b2b Compare January 10, 2023 10:08

More fixes

7b6609d

FCalabria force-pushed the new-predictive-search-api-adaptation branch from 80b0b2b to 7b6609d Compare January 10, 2023 10:09

FCalabria requested review from NathanPJF and kmeleta January 10, 2023 10:10

FCalabria and others added 3 commits January 10, 2023 11:12

Merge branch 'main' into new-predictive-search-api-adaptation

b710eca

Update 17 translation files

23eba59

Update 7 translation files

9a49543

kmeleta reviewed Jan 10, 2023

View reviewed changes

al-mcquiston closed this Jan 10, 2023

al-mcquiston reopened this Jan 10, 2023

Shopify deleted a comment from al-mcquiston Jan 11, 2023

NathanPJF added 2 commits January 10, 2023 22:38

fix text underline and centered headings

ea68a1a

additional opacity change

cc1d87a

NathanPJF requested review from kmeleta and melissaperreault January 11, 2023 05:32

NathanPJF reviewed Jan 11, 2023

View reviewed changes

NathanPJF approved these changes Jan 11, 2023

View reviewed changes

melissaperreault approved these changes Jan 11, 2023

View reviewed changes

kmeleta approved these changes Jan 11, 2023

View reviewed changes

NathanPJF merged commit 2f41ad3 into main Jan 12, 2023

NathanPJF deleted the new-predictive-search-api-adaptation branch January 12, 2023 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt predictive search layout to new api #2192

Adapt predictive search layout to new api #2192

FCalabria commented Jan 4, 2023 •

edited by NathanPJF

Loading

NathanPJF left a comment •

edited

Loading

melissaperreault Jan 5, 2023 •

edited

Loading

al-mcquiston Jan 10, 2023

melissaperreault left a comment

NathanPJF left a comment

NathanPJF Jan 5, 2023

kmeleta Jan 6, 2023

NathanPJF Jan 9, 2023

svinkle Jan 9, 2023

NathanPJF commented Jan 5, 2023 •

edited

Loading

bakura10 commented Jan 6, 2023

translation-platform bot commented Jan 9, 2023 •

edited by FCalabria

Loading

FCalabria commented Jan 9, 2023

kmeleta left a comment

kmeleta Jan 9, 2023

melissaperreault Jan 10, 2023

kmeleta Jan 10, 2023

al-mcquiston Jan 10, 2023

NathanPJF Jan 11, 2023

carolinjahn commented Jan 9, 2023

melissaperreault commented Jan 10, 2023

kmeleta Jan 10, 2023

kmeleta Jan 10, 2023

NathanPJF Jan 11, 2023

NathanPJF Jan 11, 2023

NathanPJF left a comment

melissaperreault left a comment

kmeleta left a comment

		<h3 class="predictive-search__item-heading h5">{{ product.title }}
		</h3>

	<h3 class="predictive-search__item-heading h5">{{ product.title }}
	</h3>
	<p class="predictive-search__item-heading h5">{{ product.title }}</p>

Adapt predictive search layout to new api #2192

Adapt predictive search layout to new api #2192

Conversation

FCalabria commented Jan 4, 2023 • edited by NathanPJF Loading

PR Summary:

Why are these changes introduced?

What approach did you take?

Visual impact on existing themes

Testing steps/scenarios

Demo links

Checklist

NathanPJF left a comment • edited Loading

Choose a reason for hiding this comment

melissaperreault Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melissaperreault left a comment

Choose a reason for hiding this comment

NathanPJF left a 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

NathanPJF commented Jan 5, 2023 • edited Loading

bakura10 commented Jan 6, 2023

translation-platform bot commented Jan 9, 2023 • edited by FCalabria Loading

FCalabria commented Jan 9, 2023

kmeleta left a 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

carolinjahn commented Jan 9, 2023

melissaperreault commented Jan 10, 2023

Transition

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

NathanPJF left a comment

Choose a reason for hiding this comment

melissaperreault left a comment

Choose a reason for hiding this comment

kmeleta left a comment

Choose a reason for hiding this comment

FCalabria commented Jan 4, 2023 •

edited by NathanPJF

Loading

NathanPJF left a comment •

edited

Loading

melissaperreault Jan 5, 2023 •

edited

Loading

NathanPJF commented Jan 5, 2023 •

edited

Loading

translation-platform bot commented Jan 9, 2023 •

edited by FCalabria

Loading