Skip to content

Unify keyboard events on docs.rs results #1500

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

Merged
merged 1 commit into from
Oct 8, 2021

Conversation

GuillaumeGomez
Copy link
Member

Reopening of #1452.

The keyboard events are suboptimal currently. So I improved them a bit by following what @jsha did in rustdoc. It allows to also centralize the JS in one file and remove the inline JS we currently have. Small demo below:

Peek.2021-07-20.17-26.mp4

So what it does:

  • Pressing 'S' will focus the search input (in both home page and docs.rs search result pages).
  • No event are handled in the search input (thinking about the key arrows). This is to prevent impacting accessibility.
  • If no element is focused or if a search result is focused, up/down arrows will go through the search results by setting the current one as the "active element".
  • Pressing 'esc' will blur the current active element (including the search input).

cc @Manishearth @jsha

@GuillaumeGomez
Copy link
Member Author

cc @jsha since it's very close to what you did in rustdoc search. :)

@jsha
Copy link
Contributor

jsha commented Sep 30, 2021

Could you provide a browseable demo? I don't currently have an up-to-date docs.rs env set up on my machine. I can get that working again but it might take a little while.

Also, can you confirm: This is not supposed to add new functionality, just rearrange code, right?

@GuillaumeGomez
Copy link
Member Author

I unfortunately can't, my server doesn't allow me to serve URLs with binaries... :-/

I think someone else nicely provided PRs like that. Was it you @Nemo157 by any chance?

Also, can you confirm: This is not supposed to add new functionality, just rearrange code, right?

Unification to be exact. I merge functionalities from 2 different pages. With this PR, we can now go through listed crates using arrows and on the search page we can focus on the search input using 'S'.

Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

Generally looks good to me. One meaningful typo and one small tweak.

Note that I'm not an approver on this repo so you'll need a second review from someone who is.

@GuillaumeGomez
Copy link
Member Author

Thanks @jsha for the review! It made me realize that it wasn't great so I simplified it a bit (making the code a bit longer) and added comments so it's easier to follow. Hopefully this should be much better now.

Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@jyn514 jyn514 merged commit c5d1c99 into rust-lang:master Oct 8, 2021
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.

3 participants