Skip to content

Implement Search as ARIA spec compliant* dialog and combobox pattern #2831

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

Merged
merged 5 commits into from
Feb 22, 2025

Conversation

phoneticallySAARTHaK
Copy link
Contributor

@phoneticallySAARTHaK phoneticallySAARTHaK commented Jan 20, 2025

Pretty much a complete rewrite of the UI and UI logic for search.

UI Changes:

  • fix height calculations in sectioning containers to remove extra scrollbars/overflows
  • Fixes tab order of toolbar links
  • Replaces search input with a button in toolbar, which in turn open a dialog, that has the search, which can be opened by clicking the button or pressing "/", or Ctrl+K
  • Implement search as combobox pattern (and all the changes that comes along with it)
  • Enabled virtualKeyboard API to adjust dialog height when keyboard opens in mobile (currently only chromium browsers support it)
  • Removes the ready class from search element.
  • Updat color variable to satisfy WCAG level AA

More details are in commit messages. I haven't updated the changelog, because I don't know what's relevant.

Visual Regression script probably won't detect the change?

*Mostly. There might be minor variations

@Gerrit0 Gerrit0 mentioned this pull request Jan 25, 2025
15 tasks
Copy link
Collaborator

@Gerrit0 Gerrit0 left a comment

Choose a reason for hiding this comment

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

A couple general issues:

  1. With no search history, typing something and then clearing the search field results in the claim "No Recent Searches"... which is rather confusing, I just searched for something! Didn't click on anything, but... maybe that's expected, but it greatly confused me for a bit.
  2. This introduced some contrast issues in the new search dialog (run chromium's lighthouse checks)

This is looking really awesome! It seems to behave significantly better on mobile and I like the change on desktop as well

@Gerrit0 Gerrit0 added this to the v0.28.0 milestone Jan 25, 2025
@phoneticallySAARTHaK
Copy link
Contributor Author

Regarding the recent searches message, yup, totally agree, I think even left a comment about in the code that it's actually search results. I picked up it up from remix.run, and I'm confused what to show there, "no recent... What?"

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jan 25, 2025

Would it be better to just not show anything there? It's a different dialog size, but just showing a search bar the first time it is opened doesn't seem completely wrong to me.

I also just realized that storing IDs is not necessarily the way we should go about that. IDs are somewhat likely to change between rebuilds of the site as they are dependent on conversion order, so if the very first class TypeDoc converts gets a new property, all the IDs will end up shifted.

@phoneticallySAARTHaK
Copy link
Contributor Author

phoneticallySAARTHaK commented Jan 25, 2025

Edit: Removing it.

... Yup, I mainly added for cosmetic UX purposes. It's not important and I already had doubts about it (that's why it's the last commit).

Without anything it feels.. empty. I'll get some quick design advice on this, otherwise I'll just remove it.

And yes I tested the change of IDs, I still added it on the premises that atleast it won't crash.

From commit message:

  • This implementation prevents broken links, but refs might change, changing the suggestions

It was the simplest way to achieve it, and I don't wanna make it complicated, so I'll probably just remove it. But I don't think occasional changing suggestions is a big deal. But after all, it is a bug.

So here's my confusion:

  • last search is mainly for UX, to not have a tiny search dialog
  • The message "no recent search" is a bit confusing, it should probably be changed.
  • it could be a useful feature, but current implementation has a minor bug.
  • That shouldn't be a big issue, and a lot of people probably wouldn't even notice.

Tradeoffs everywhere.. What are your thoughts about it?

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Feb 9, 2025

I've merged this into the beta branch

@Gerrit0 Gerrit0 changed the base branch from master to beta February 22, 2025 03:24
It will now behave correctly if a custom theme overrides `getReflectionClasses` to include additional non-filter classes.
@Gerrit0 Gerrit0 merged commit 2b407ab into TypeStrong:beta Feb 22, 2025
7 of 8 checks passed
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Feb 22, 2025

Thank you!

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