Skip to content
This repository has been archived by the owner on Jun 11, 2021. It is now read-only.

Adds a query prop #51

Closed
wants to merge 1 commit into from
Closed

Adds a query prop #51

wants to merge 1 commit into from

Conversation

orta
Copy link

@orta orta commented Jun 22, 2020

Summary

I've been looking at integrating the react version of the docsearch into the TS website in lieu of algolia/docsearch#955 in this PR microsoft/TypeScript-Website#690

The bug in the text to speech isn't present in the new version (the HTML is simpler, and contains the full text of each result )- which solves that issue, but I need to be able to pass an initial query into the modal, so that I can have someone type in the input and then send that off.

2020-06-22 10-54-45 2020-06-22 10_56_18


That said, I'm not too sure if this is the right way to do it vs all the useX functions that are around but at least it proves the concept

`hierarchy.lvl5:${snipetLength.current}`,
`hierarchy.lvl6:${snipetLength.current}`,
`content:${snipetLength.current}`,
`hierarchy.lvl1:${snippetLength.current}`,
Copy link

Choose a reason for hiding this comment

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

good catch!

@francoischalifour
Copy link
Owner

@orta Thanks for the contribution! Before reviewing this PR, could you please let us know why you decided not to use the default DocSearchButton?

@orta
Copy link
Author

orta commented Jun 24, 2020

It doesn't suite the website style - I'm OK with the modal looking out of place, but not a top nav design element

@Shipow
Copy link
Collaborator

Shipow commented Jun 25, 2020

Hi Orta,
thanks for you interest and participation to DocSearch.
I totally understand the problem of styling regarding the search button, it looks like we didn't anticipated a full colored nav-bar, neither the full block styling. We'll fix that before the release.
However we did develop this version of DocSearch having in mind to replace the search field in the navigation bar by a button, the behavior expected is to have results displayed after each keystroke while benefiting of a wilder area to expose the information and less visual charge from the rest of the website. (+ consistent ux across devices).
I feel that by switching back the button to a search field, the interaction and behavior of DocSearch are kind of broken.

As I'm very interested by the developer experience that our user will have and report with this release I'd like to know if moving back to the button is an option for you. If we were to allow a generic dark feel to the search button, would you be more likely to use it? And if if not I'd really appreciate if you could elaborate on the reasons to let us adapt our product.

@orta
Copy link
Author

orta commented Jun 25, 2020

Is moving to the button an option

Probably not, but I'm open to seeing what it looks like though and could change my mind, the design of our site is pretty set in stone at this point. 👍


I totally see what you're going for as a button. Makes sense, and when you use the mouse it does the popover straight away, but I can't make that happen when you do focus via the keyboard (you can't tab through the page then) so I could either open the modal when there's a keypress which might lose characters (and would still need this PR) or I can send a keyboard user when they press return (which was my current implementation)

https://typescript-v2-690.vercel.app if you want to try

@francoischalifour
Copy link
Owner

open the modal when there's a keypress

That's a great point you have here @orta, and we decided to implement this feature because it makes sense on a UX point of view.

You can check out #54 and let us know if that answers your initial request.

@Shipow
Copy link
Collaborator

Shipow commented Jul 2, 2020

Thanks a lot @orta for raising this concern, I'm glad you catched this before any release! <3

@orta
Copy link
Author

orta commented Jul 20, 2020

🎉 thanks folks

francoischalifour added a commit that referenced this pull request Sep 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants