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: Improve keyboard interactions in searchbox #6537

Merged
merged 20 commits into from
Apr 11, 2024

Conversation

Baccega
Copy link
Contributor

@Baccega Baccega commented Mar 24, 2024

Description

This PR is improving the UX of the searchbox by adding interactivity with the up/down arrows and enter keys, to mimic the behaviour of similar searchboxes.
While developing the feature I've also found a bug in the useKeyboardCommands (the eventListener wasn't properly cleaned) that was causing issues to my addition.

Validation

I've validated it locally by setting some random search results from prod into the react state, a video of the feature in action is available here

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npx turbo format to ensure the code follows the style guide.
  • I have run npx turbo test to check if all tests are passing.
  • I have run npx turbo build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

@Baccega Baccega requested a review from a team as a code owner March 24, 2024 23:57
Copy link

vercel bot commented Mar 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Apr 10, 2024 1:51pm

@Baccega Baccega changed the title Improve keyboard interactions in searchbox feat: Improve keyboard interactions in searchbox Mar 25, 2024
@AugustinMauroy AugustinMauroy added the github_actions:pull-request Trigger Pull Request Checks label Mar 30, 2024
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Mar 30, 2024
Copy link
Contributor

github-actions bot commented Mar 30, 2024

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 98 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 99 🟢 98 🟢 100 🟢 92 🔗
/en/download 🟢 98 🟢 100 🟢 100 🟢 91 🔗
/en/blog 🟢 99 🟢 100 🟢 96 🟢 92 🔗

Copy link
Contributor

github-actions bot commented Mar 30, 2024

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 91%
90.1% (583/647) 76.08% (175/230) 92.12% (117/127)

Unit Test Report

Tests Skipped Failures Errors Time
128 0 💤 0 ❌ 0 🔥 5.388s ⏱️

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

Cool improvement

components/Common/Search/utils.ts Outdated Show resolved Hide resolved
@Baccega
Copy link
Contributor Author

Baccega commented Mar 30, 2024

While implementing the feedbacks I've improved the feature a little bit further by bounding the selection (no overflowing now) and adding a scrollIntoView effect

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

LGTM ! Super cool first contribution 🎉

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

We shouldn't use JavaScript to achieve this. This can easily be done with pure HTML and the proper aria-* tags (and other attributes such as tabindex, role, etc)

I appreciate the invested time in this, but unfortunately we shouldn't try to solve accessibility with JavaScript.

@Baccega
Copy link
Contributor Author

Baccega commented Apr 1, 2024

We shouldn't use JavaScript to achieve this. This can easily be done with pure HTML and the proper aria-* tags (and other attributes such as tabindex, role, etc)

I appreciate the invested time in this, but unfortunately we shouldn't try to solve accessibility with JavaScript.

Do you mean that we shouldn't have this behaviour at all? MDN Web Docs has this feature (With all the proper accessibility tags) and it does not work without js

In that case I can revert the feature and just leave the fix of the useKeyboardCommands hook, otherwise I can imitate the MDN searchbox accessibility features (with aria-selected, role and aria-activedescendant)

@ovflowd
Copy link
Member

ovflowd commented Apr 1, 2024

We shouldn't use JavaScript to achieve this. This can easily be done with pure HTML and the proper aria-* tags (and other attributes such as tabindex, role, etc)

I appreciate the invested time in this, but unfortunately we shouldn't try to solve accessibility with JavaScript.

Do you mean that we shouldn't have this behaviour at all? MDN Web Docs has this feature (With all the proper accessibility tags) and it does not work without js

In that case I can revert the feature and just leave the fix of the useKeyboardCommands hook, otherwise I can imitate the MDN searchbox accessibility features (with aria-selected, role and aria-activedescendant)

Interesting. My understanding is that navigation via tab key is sufficient for the a11y standard.

That's because, to my understanding, there's no sense of what is left and right, up and down.

I believe if we make navigation through tab keys work (which is possible with plain html) we could content with that.

Navigation with arrow keys indeed requires JavaScript, but I wonder if we should even address that. If that is indeed the goal, I still believe the proposed patch can be simplified and no Math calculations are needed.

You can do it with a simple state that stores the current index (default 0)

And have an if statement (if the next index < length of results) then update the state, otherwise do nothing.

@Baccega
Copy link
Contributor Author

Baccega commented Apr 1, 2024

We shouldn't use JavaScript to achieve this. This can easily be done with pure HTML and the proper aria-* tags (and other attributes such as tabindex, role, etc)

I appreciate the invested time in this, but unfortunately we shouldn't try to solve accessibility with JavaScript.

Do you mean that we shouldn't have this behaviour at all? MDN Web Docs has this feature (With all the proper accessibility tags) and it does not work without js
In that case I can revert the feature and just leave the fix of the useKeyboardCommands hook, otherwise I can imitate the MDN searchbox accessibility features (with aria-selected, role and aria-activedescendant)

Interesting. My understanding is that navigation via tab key is sufficient for the a11y standard.

That's because, to my understanding, there's no sense of what is left and right, up and down.

I believe if we make navigation through tab keys work (which is possible with plain html) we could content with that.

Navigation with arrow keys indeed requires JavaScript, but I wonder if we should even address that. If that is indeed the goal, I still believe the proposed patch can be simplified and no Math calculations are needed.

You can do it with a simple state that stores the current index (default 0)

And have an if statement (if the next index < length of results) then update the state, otherwise do nothing.

I've updated the PR with the simplified logic you suggested and imitated the searchbox behaviour of MDN (which I found out that implements the combobox pattern) 👍🏻

@ovflowd
Copy link
Member

ovflowd commented Apr 2, 2024

We shouldn't use JavaScript to achieve this. This can easily be done with pure HTML and the proper aria-* tags (and other attributes such as tabindex, role, etc)

I appreciate the invested time in this, but unfortunately we shouldn't try to solve accessibility with JavaScript.

Do you mean that we shouldn't have this behaviour at all? MDN Web Docs has this feature (With all the proper accessibility tags) and it does not work without js
In that case I can revert the feature and just leave the fix of the useKeyboardCommands hook, otherwise I can imitate the MDN searchbox accessibility features (with aria-selected, role and aria-activedescendant)

Interesting. My understanding is that navigation via tab key is sufficient for the a11y standard.
That's because, to my understanding, there's no sense of what is left and right, up and down.
I believe if we make navigation through tab keys work (which is possible with plain html) we could content with that.
Navigation with arrow keys indeed requires JavaScript, but I wonder if we should even address that. If that is indeed the goal, I still believe the proposed patch can be simplified and no Math calculations are needed.
You can do it with a simple state that stores the current index (default 0)
And have an if statement (if the next index < length of results) then update the state, otherwise do nothing.

I've updated the PR with the simplified logic you suggested and imitated the searchbox behaviour of MDN (which I found out that implements the combobox pattern) 👍🏻

Appreciate it! Reviewing now 👀

@Baccega
Copy link
Contributor Author

Baccega commented Apr 10, 2024

Besides my final comment, LGTM! Awesome stuff. Thank you so much for bearing with us and doing all these changes ❤️

Happy to contribute 😄

If there's only two changes I could ask for is:

  • Override the Tabs styles to look like the previous styles
    image

I've now update the tabs to include a "secondaryLabel" option, similar to the previous style

Screenshot 2024-04-10 at 13 52 19

Screenshot 2024-04-10 at 13 52 27

  • When there's no search query, show all results from the selected tab (or at least follow the logic of showing the fist X results)

I've removed the empty ("Search something...") state, but I'm not sure if the results will actually show on production because they depend on how the search with an empty term will behave (locally I've use mock data to develop the feature).
Do you have any suggestion/do you know if the search returns something with an empty search term?

I think that'd be great probably?

@ovflowd
Copy link
Member

ovflowd commented Apr 10, 2024

I've removed the empty ("Search something...") state, but I'm not sure if the results will actually show on production because they depend on how the search with an empty term will behave (locally I've use mock data to develop the feature).
Do you have any suggestion/do you know if the search returns something with an empty search term?

@micheleriva is there a way folks can have a test database locally? 🤔

@ovflowd
Copy link
Member

ovflowd commented Apr 10, 2024

I also guess you can always wait for the Vercel deployment to test things out :pokerface:

@micheleriva
Copy link
Contributor

@ovflowd you can set the environment variables for local development!

@ovflowd
Copy link
Member

ovflowd commented Apr 10, 2024

@ovflowd you can set the environment variables for local development!

That is not the issue. I don't want to share our api keys publicly. Is there a way folks can test things locally without using our keys?

@micheleriva
Copy link
Contributor

@ovflowd I understand, but keep in mind that ORAMA_CLOUD_ENDPOINT and ORAMA_CLOUD_API_KEY are commit-safe and can be shared publicly. These are the only two variables you could share.

@ovflowd ovflowd added this pull request to the merge queue Apr 11, 2024
Merged via the queue into nodejs:main with commit 6581249 Apr 11, 2024
15 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.

6 participants