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

Search bar navigation #1088

Merged
merged 11 commits into from
Mar 21, 2024
Merged

Conversation

EmileTrotignon
Copy link
Collaborator

@EmileTrotignon EmileTrotignon commented Feb 27, 2024

The odoc search bar does have navigation : you cannot use the arrow keys to select a search result.

This add javascript that allows this.

There is also a shortcut to enter search : / and to exit : escape.
Escape is very intuitive, an / is also used by other website like github or discuss.

There should no diff on css, as the js uses the focus mechanism and there was css for that already.

I am thinking of adding a visual cue that / focuses the search bar. Github has one, but I am not very happy with what I came up with. Ideas and help are welcome.

@panglesd
Copy link
Collaborator

panglesd commented Mar 4, 2024

Thanks, that a much improved user experience!

Small feedback from using it:

  • Going "up" when already at the top yields an error:
    Uncaught TypeError: results[(current_focus - 1)] is undefined
    
  • (nitpick) It would be nice if the focus were teleported to the input when typing again:
    • Type "abc"
    • Type "down arrow" several times... don't find what was looked for
    • type "d", refocuses on the search bar (and add "d").
  • One simple way to tell about the keybinding is to use the placeholder: Turn "🔎 Search..." into "🔎 Type '/' to search" (just like in github). I find it fine.

@EmileTrotignon
Copy link
Collaborator Author

@panglesd

Going "up" when already at the top yields an error:

This is fixed

(nitpick) It would be nice if the focus were teleported to the input when typing again:

This is done as well

One simple way to tell about the keybinding is to use the placeholder: Turn "🔎 Search..." into "🔎 Type '/' to search" (just like in github). I find it fine.

I wanted to be too much like github : my attempt was "🔎 Type [/] to search" which looked ugly. This is now done, thanks.

function enter_focus() {
if (n_focus === null) return;
let elt = current_result();
elt.click();
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This function seems unused

Comment on lines 144 to 148
&& (ascii >= 65 && ascii <= 90) // lowercase letter
|| (ascii >= 97 && ascii <= 122) // uppercase letter
|| (ascii >= 48 && ascii <= 57) // numbers
|| (ascii >= 32 && ascii <= 46) // ` ` to `.`
|| (ascii >= 58 && ascii <= 64) // `:` to `@`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we want that all those ascii characters trigger search...

Copy link
Collaborator

@panglesd panglesd Mar 19, 2024

Choose a reason for hiding this comment

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

(nitpick) It would be nice if the focus were teleported to the input when typing again:

This is done as well

Ah, maybe this was due to this! But typing on any ascii character even outside of search triggers search, maybe that is too much?

Copy link
Collaborator

@panglesd panglesd 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! Thanks!

@panglesd panglesd merged commit 32506df into ocaml:master Mar 21, 2024
9 of 10 checks passed
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.

None yet

2 participants