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

feat: adding a skip to search anchor #850

Merged
merged 3 commits into from
Dec 4, 2023

Conversation

ivan-demchenko
Copy link
Contributor

@ivan-demchenko ivan-demchenko commented Nov 23, 2023

UPD: due to accessibility-related reasons, the autofocus attribute idea was parked. Instead, this PR adds the "Skip to search" anchor.

Original:
This PR is a suggestion to add the autofocus attribute to the search input. The main driver is that when opening the docs.npmjs.com page, a user needs to set the focus in the input manually to start searching.

There are, however, some accessibility trade-offs to consider. Therefore, I am open to any feedback and outcome.

@ivan-demchenko ivan-demchenko force-pushed the feat/autofocus-search-input branch 2 times, most recently from 13cf0a2 to 999de1d Compare November 23, 2023 14:52
@lukekarrys
Copy link
Contributor

lukekarrys commented Nov 24, 2023

Due to the accessibility tradeoffs, I think it might be better to add "Search" to the skip nav. That way the initial <tab> will open the skip nav with two options: skip to content and skip to search. Selecting skip to search will focus the input.

Thoughts on that?

@ivan-demchenko
Copy link
Contributor Author

Hi @lukekarrys
Yeah, I think this is a better approach. I'll update the PR

@ivan-demchenko ivan-demchenko force-pushed the feat/autofocus-search-input branch from 999de1d to 6aea9ea Compare November 29, 2023 13:48
@ivan-demchenko ivan-demchenko force-pushed the feat/autofocus-search-input branch from 6aea9ea to 21cfb0a Compare November 29, 2023 13:49
@ivan-demchenko ivan-demchenko changed the title feat: enable autofocusing search input on desktop feat: adding a skip to search anchor Nov 29, 2023
@lukekarrys
Copy link
Contributor

I like this a lot! Do you think the search input should also be focused after Skip to Search is selected?

@lukekarrys lukekarrys requested a review from a team as a code owner November 29, 2023 21:35
@lukekarrys
Copy link
Contributor

@ivan-demchenko I've been meaning to restyle the SkipBox so I just pushed a commit that restyles that and also tries to use the Gatsby onRouteUpdate handler to automatically focus the search input when the hash is set to #search-input-box. Let me know what you think!

@lukekarrys
Copy link
Contributor

@ivan-demchenko There's also the issue of needing to open the search drawer on smaller screens when skipping to the search. I might try to come up with a solution for that in the next couple days. But feel free to try that out as well if you want.

@ivan-demchenko
Copy link
Contributor Author

ivan-demchenko commented Nov 30, 2023

@lukekarrys the new styling looks great, thank you! Also, I think, the input should be focused when "Skip to search" has been selected.

I noticed that on Firefox on Mac, the first tab doesn't trigger the "Skip to ..." link to focus. The focus jumps straight to the search bar. This behaviour can be observed on Safari too. I found how to "fix" (here or here) the problem, after which Firefox started behaving as expected. But I don't believe the users will ever try to change their system settings to make the "Skip to..." feature work for them.

@ivan-demchenko
Copy link
Contributor Author

ivan-demchenko commented Nov 30, 2023

As for opening the search box on the mobile, I'd use CSS for that (instead of JS and React hooks). The Search "button" (with the 🔎 icon) can be a Label with the input inside. Then, with :focus-within we could show/hide the elements as we see fit, here's a quick PoC. What do you think, @lukekarrys?

@lukekarrys
Copy link
Contributor

@ivan-demchenko That PoC looks really nice and very clean! I like it a lot. I'm hesitant to include something like that in this PR since it would likely require undoing a lot of the current React hooks implementation of this. I'd be in favor of including that in a future PR if you would like, in order to keep the scope of this one small.

Considering that there is no way to focus the search at all currently, I think this is shippable without mobile support for now. What do you think?

@lukekarrys lukekarrys merged commit e388aee into npm:main Dec 4, 2023
8 of 9 checks passed
@ivan-demchenko
Copy link
Contributor Author

@lukekarrys Thank you very much for merging the PR, this is very exciting!

Oh, absolutely! I think we should take small steps and improve things incrementally, I've never been a fan of big-bangs.

As per the PoC, I agree with you, this would be a big change. You listed the cons correctly. As per the pros, it would reduce the number of dependencies on the JS libraries, however, resulting in a smaller bundle. But anyway, this is for the next time.

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.

2 participants