Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate DocSearch #2016

Closed
wants to merge 5 commits into from
Closed

Integrate DocSearch #2016

wants to merge 5 commits into from

Conversation

solimant
Copy link
Collaborator

@solimant solimant commented Jun 14, 2021

Closes #1999

✅ Pull Request Checklist:

  • 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).
  • 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

@snowystinger
Copy link
Member

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?

@solimant
Copy link
Collaborator Author

solimant commented Jun 16, 2021

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.

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".
  • 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
    • maybe would allow us to do infinite scroll/list more results than the 5?

@solimant
Copy link
Collaborator Author

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.

@snowystinger
Copy link
Member

@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

@solimant
Copy link
Collaborator Author

@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.

@majornista
Copy link
Collaborator

@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
Copy link
Collaborator

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

@snowystinger
Copy link
Member

@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

@devongovett
Copy link
Member

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.

@solimant
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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.

Update to adobe#2016 to improve accessibility in DocSearch results so that they better implement the WAI-ARIA ComboBox design pattern.

- Fixes a few other docs issues identified with aXe DevTools.
- Fixes a couple console error messages with themeSwitcherButton and hamburgerButton.
@majornista
Copy link
Collaborator

Issue filed with Algolia: algolia/docsearch#1014

@Shipow
Copy link

Shipow commented Jun 25, 2021

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

@majornista
Copy link
Collaborator

majornista commented Jun 28, 2021

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.

@devongovett
Copy link
Member

Superseded by #3383

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Would like to be able to search the docs
6 participants