-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add settings search and dialog refinements #10611
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
base: main
Are you sure you want to change the base?
Conversation
Reviewed the new exclusion mechanism commit. The implementation is clean with good test coverage. No new issues found.
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
Reviewed the new exclusion mechanism commit. The implementation is clean with good test coverage. No new issues found.
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
| } | ||
| } | ||
|
|
||
| const defaultExtensionState = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually try to avoid adding tests that require on having all of the state like this, since it leads to more stuff we need to update when adding unrelated things to extension state. Honestly there's so much mocking in this test that it might be better just to delete it.
| return [] | ||
| } | ||
|
|
||
| // Normalize query to lowercase for case-insensitive matching |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not blocking feedback, but we have the fzf library - maybe we can use it here?
|
|
||
| /** | ||
| * Valid section names that correspond to tabs in SettingsView. | ||
| * Defined locally to avoid circular dependencies with SettingsView.tsx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would really be nice not to duplicate this here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we try to unify this logic more with the settingsview logic?
| examples?: string[] | ||
| } | ||
|
|
||
| const SEARCH_EXCLUSIONS: ExclusionRule[] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if instead of doing this, we can add a data-searchable property on the div where we display the information. I haven't thought deeply about it but that seems like it would be more elegant and maintainable.
Summary
Important
Adds settings search with keyboard navigation and highlights, refines UI, and updates components and tests for new functionality.
SettingsViewto jump to sections and highlight matches.SettingsSearchInputandSettingsSearchResultscomponents.SettingsViewto integrate search functionality.SettingsSearchInputandSettingsSearchResults.parseSettingsI18nKeys.spec.tsandsettingsSearchExclusions.spec.tsfor new exclusions and parsing logic.index.cssfor search navigation.settings.jsonfiles.parseSettingsI18nKeys.tsto exclude non-actionable settings.This description was created by
for f4abc87. You can customize this summary. It will automatically update as commits are pushed.