react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

Integrate DocSearch

Open solimant opened this issue 4 years ago • 14 comments

Closes #1999

✅ Pull Request Checklist:

  • [X] Included link to corresponding React Spectrum GitHub Issue.
  • [ ] Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • [X] Filled out test instructions.
  • [ ] Updated documentation (if it already exists for this component).
  • [ ] Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Light

image

Dark

image

Mobile

image

🧢 Your Project:

Adobe/RSP

solimant avatar Jun 14 '21 08:06 solimant

This is more a general question, do we want it to be the first thing that Tab lands on when you open the page? It seems to make sense to me that way, allows a keyboard user to very quickly navigate to the thing they want. They don't have to go through the whole navigation on the side.

I haven't found any issues with it yet, how frequently does it update?

snowystinger avatar Jun 16 '21 19:06 snowystinger

This is more a general question, do we want it to be the first thing that Tab lands on when you open the page? It seems to make sense to me that way, allows a keyboard user to very quickly navigate to the thing they want. They don't have to go through the whole navigation on the side.

Thank you, Rob. I think both behaviors are fine, though I would expect a keyboard user would want to type something rather than tabbing through the nav.

I haven't found any issues with it yet, how frequently does it update?

Algolia updates their index daily.

solimant avatar Jun 16 '21 20:06 solimant

Pretty nice, seems to work well! I'm happy for this to go in as is, just gonna list some things that I noted/future work perhaps that can be done.

  • Voiceover/Talkback had mixed success announcing the contents of each item in the popover. Voiceover was pretty decent but would not announce the full contents of the highlighted item occasionally/would merge the left/right text (the text separated by the divider). Talkback wouldn't announce the text at all, just stating "link to the result, in list item x of 0".

Thank you, Daniel. Since the drop down is totally managed by DocSearch as a component, it could be a limitation there. So I'm interested to know if this is an isolated experience in our integration, or indeed a general limitation that shows on other DocSearch client websites as well; e.g. React and TypeScript

  • Would be nice if we could leverage the data returned by Algolia but use our own components (e.g. use a tray instead of a popover for mobile). That way we could address some of the accessibility stuff above as well

I think this would be super ideal, and would give us all the power we might be looking for, though I doubt we can use something other than their autocomplete dropdown.

  • maybe would allow us to do infinite scroll/list more results than the 5?

I think this is possible by setting the hitsPerPage option in algoliaOptions. More options here.

solimant avatar Jun 17 '21 02:06 solimant

@solimant @LFDanLu I did find this https://www.algolia.com/doc/ui-libraries/autocomplete/guides/creating-a-renderer/ so this might be something we could do in the future to handle the complete rendering ourselves

snowystinger avatar Jun 17 '21 04:06 snowystinger

@solimant @LFDanLu I did find this https://www.algolia.com/doc/ui-libraries/autocomplete/guides/creating-a-renderer/ so this might be something we could do in the future to handle the complete rendering ourselves

@snowystinger - From what I understand, I think beyond CSS styling, we're bound to use their dedicated search dropdown UI. That said, they do support some additional dropdown behavior, but nothing as extensive as completely replacing their dropdown UI.

solimant avatar Jun 17 '21 04:06 solimant

@solimant I agree with @snowystinger that the default Algolia search results have accessibility problems, for example each returned result in the listbox is announced as "Link to the result" due to a hard coded aria-label on each link in the template for suggestions: https://github.com/algolia/docsearch/blob/7c1d293af9b828cf2553d7ea61c54eb78f4dc882/src/lib/templates.js#L13. One can't distinguish between the suggestions.

majornista avatar Jun 17 '21 14:06 majornista

Also, if we ever wanted to localize our docs, with these hard-coded strings, Algolia clearly hasn't considered that.

majornista avatar Jun 17 '21 14:06 majornista

@majornista i think with customizing the renders we can get that and not blocking, but I did find this as well https://community.algolia.com/algoliasearch-helper-js/ so I believe we can fully customize any rendering in the future

snowystinger avatar Jun 17 '21 17:06 snowystinger

Looks like their docsearch uses this API client under the hood: https://www.npmjs.com/package/algoliasearch. IMO it would be better if we used our own components for this. We put a lot of time and effort into testing and making our ComboBox accessible, plus it would be a nice showcase.

devongovett avatar Jun 17 '21 19:06 devongovett

@solimant I agree with @snowystinger that the default Algolia search results have accessibility problems, for example each returned result in the listbox is announced as "Link to the result" due to a hard coded aria-label on each link in the template for suggestions: https://github.com/algolia/docsearch/blob/7c1d293af9b828cf2553d7ea61c54eb78f4dc882/src/lib/templates.js#L13. One can't distinguish between the suggestions.

@majornista - there seems to have been a PR that addresses that, but it never saw the light. We should bring it up again.

solimant avatar Jun 17 '21 19:06 solimant

Looks like their docsearch uses this API client under the hood: https://www.npmjs.com/package/algoliasearch. IMO it would be better if we used our own components for this. We put a lot of time and effort into testing and making our ComboBox accessible, plus it would be a nice showcase.

@devongovett - fair point. I'll look into the API.

solimant avatar Jun 17 '21 20:06 solimant

Issue filed with Algolia: https://github.com/algolia/docsearch/issues/1014

majornista avatar Jun 24 '21 20:06 majornista

Hello, I see you've started to integrate DocSearch, and wanted to make sure you had the opportunity to check out the new version: https://github.com/algolia/docsearch/tree/next

Shipow avatar Jun 25 '21 02:06 Shipow

Hello, I see you've started to integrate DocSearch, and wanted to make sure you had the opportunity to check out the new version: https://github.com/algolia/docsearch/tree/next

@Shipow New alpha version throws an error when we try interact with the search box, and we are unable to debug because the source code is minified. Also url to import in your documentation for alpha version is wrong. Should be https://cdn.jsdelivr.net/npm/@docsearch/js@alpha and https://cdn.jsdelivr.net/npm/@docsearch/css@alpha.

majornista avatar Jun 28 '21 18:06 majornista

Superseded by #3383

devongovett avatar Apr 12 '23 14:04 devongovett