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.

Sign up for GitHub

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

Jump to bottom

Support type '/' to search #123355

Merged
merged 1 commit into from
Apr 16, 2024
Merged

Support type '/' to search #123355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/librustdoc/html/static/js/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,7 @@ function preLoadCss(cssUrl) {

case "s":
case "S":
case "/":
ev.preventDefault();
searchState.focus();
break;
Expand Down Expand Up @@ -1310,7 +1311,7 @@ function preLoadCss(cssUrl) {

const shortcuts = [
["?", "Show this help dialog"],
["S", "Focus the search field"],
["S / /", "Focus the search field"],
["↑", "Move up in search results"],
["↓", "Move down in search results"],
["← / →", "Switch result tab (when results focused)"],
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/html/templates/page.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ <h2>Files</h2> {# #}
aria-label="Run search in the documentation" {#+ #}
autocomplete="off" {#+ #}
spellcheck="false" {#+ #}
placeholder="Click or press ‘S’ to search, ‘?’ for more options…" {#+ #}
placeholder="Type ‘S’ or ‘/’ to search, ‘?’ for more options…" {#+ #}
Copy link
Contributor

@notriddle notriddle Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we change this to just "Search"?

Suggested change
placeholder="Type ‘S’ or ‘/’ to search, ‘?’ for more options…" {#+ #}
placeholder="Search" {#+ #}

I know it's a bit off-topic, and I'll approve this PR even if the change isn't made, but the keyboard command should probably be made less prominent.

  • The whole premise of the PR is that we don't actually need to tell people about this keyboard command. They will assume it exists in Rustdoc because it exists in GitHub, Discourse, MDN, Zulip, GitLab, other websites that I don't know about, and VI.
  • If you don't have a hardware keyboard, the UI is presenting things to you that you can't use.
  • The ? command only works if you're on a big enough screen. If you're on a small viewport, the ? keyboard command does nothing even if you have a hardware keyboard. The button is also gone.
  • "Search" ought to be the first word, for ease of scanning.

Copy link
Member

@GuillaumeGomez GuillaumeGomez Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking about adding the available keys on the right of the search input a bit like what the github search input has. Then depending on the screen width, we can easily hide the ones not available.

Copy link
Contributor

@notriddle notriddle Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The search bar has had explicit words about the keyboard commands since 1.0. I'm pretty sure the reason they did it that way was because the ? button didn't exist yet.

Now that there's a help button, do we really need the stuff in the search box? We don't have on-screen indicators for most keyboard shortcuts; what makes search different other than the fact that we've always done it that way?

Ranked preferences

  1. No indicator for keyboard shortcuts unless you open the Help window is ideal IMO. Keyboard shortcuts aren't important enough to justify redundancy. We should spend our above-the-fold-clutter budget on the actual crate's documentation.

  2. If you really want to have an on-screen indicator for this keyboard shortcut, the way it's done now is ideal. It's very straightforward, so at least we're spending screen real estate on something the user will actually understand and benefit from.

  3. Are you suggesting something like this?

    image

    I don't like that approach at all. It seems like a bad compromise that's worse than either of the "extremes," because it's adding stuff on the screen but it's so subtle that new readers probably won't even understand what it means. Few sites display it that way according to this guy

Copy link
Member

Choose a reason for hiding this comment

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

Convenience and also people (sadly) seem to rarely ever click on the ? button.

Copy link
Member

Choose a reason for hiding this comment

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

I think maybe just having "/ to search" as a short snippet would be fine. We shouldn't inundiate them with information here, but having "/ to search" on a search box is quite standard and not distracting (github does it).

Having more complex text is distracting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with just having '/' to search or Type '/' to search, like what github does

type="search"> {# #}
<div id="help-button" tabindex="-1"> {# #}
<a href="{{page.root_path|safe}}help.html" title="help">?</a> {# #}
Expand Down
Loading