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

Unify keyboard events on docs.rs results #1452

Merged
merged 1 commit into from
Sep 12, 2021

Conversation

GuillaumeGomez
Copy link
Member

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

@syphar
Copy link
Member

syphar commented Jul 21, 2021

@GuillaumeGomez I would prefer to leave the review with someone knowing more JS than I do,

though if this takes too much time I would at least dig into it and validate it works as it should :)

@GuillaumeGomez
Copy link
Member Author

It was more to get an "overall" review. There are DOM changes after all. ;)

templates/core/home.html Outdated Show resolved Hide resolved
templates/releases/releases.html Outdated Show resolved Hide resolved
@GuillaumeGomez GuillaumeGomez force-pushed the keyboard-events branch 3 times, most recently from cd74054 to 490b360 Compare August 6, 2021 17:40
@GuillaumeGomez
Copy link
Member Author

Updated!

@GuillaumeGomez
Copy link
Member Author

cc @Nemo157

@jyn514
Copy link
Member

jyn514 commented Aug 21, 2021

The keyboard events are suboptimal currently. So I improved them a bit by following what jsha did in rustdoc.

Can you expand on this? What does this PR actually do?

@GuillaumeGomez
Copy link
Member Author

Instead of simply highlighting an item with a CSS class, we focus it. Meaning that if you press enter, you go to the URL (and if you press ctrl+enter it opens it in another tab).

The other change is that pressing 's' is now focusing on the search input of the page (like in rustdoc). Just to be clear: this js isn't on the rustdoc pages so there is no "conflict" between the two in any way. It's on the search pages and the home page of docs.rs.

@jyn514
Copy link
Member

jyn514 commented Aug 21, 2021

@GuillaumeGomez this breaks autocomplete for the search box unfortunately:

docs.rs.selection.mp4

@GuillaumeGomez
Copy link
Member Author

Just like on rustdoc. Question then: should I disable it on the home page then or should I disable the autocomplete?

@jyn514
Copy link
Member

jyn514 commented Aug 22, 2021

Hmm, it seems unfortunate this is broken for rustdoc to.

@Manishearth @jsha do one of you have opinions on what to do here? I kind of lean towards keeping the browser's default behavior.

@Manishearth
Copy link
Member

Strongly lean towards not overriding browser default behavior

@GuillaumeGomez
Copy link
Member Author

I'll disable this functionality on the home page then.

@GuillaumeGomez
Copy link
Member Author

I disabled the usage of arrows on the keyboard (leaving only the escape key).

So that means:

  • If that if nothing is selected and you press up/down arrow, it'll go through the list.
  • If you press ESC, it'll blur the current active element
  • If you press 'S' and you're not in the search input, it'll focus on it.

@GuillaumeGomez
Copy link
Member Author

Does it look good to everyone?

@GuillaumeGomez
Copy link
Member Author

Ping?

@GuillaumeGomez
Copy link
Member Author

Considering it's an improvement as it's centralize and make events handling coherent and that all comments were resolved, I'll merge it for the time being then. :)

@GuillaumeGomez GuillaumeGomez merged commit 152afdd into rust-lang:master Sep 12, 2021
@GuillaumeGomez GuillaumeGomez deleted the keyboard-events branch September 12, 2021 20:09
@jyn514
Copy link
Member

jyn514 commented Sep 21, 2021

@GuillaumeGomez can you please avoid merging PRs without review? It wouldn't have gone out until today anyway, and I don't like bypassing feedback. Now if I find something wrong I have to delay the deploy until this is reverted.

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.

4 participants