-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Refactor preference UI + add 'Commonly Used' section #9439
Refactor preference UI + add 'Commonly Used' section #9439
Conversation
0c9cd65
to
82b4442
Compare
fb8bac1
to
76e1cb1
Compare
this.lastSearchedLiteral = newSearchTerm; | ||
this.lastSearchedFuzzy = newSearchTerm.replace(/\s/g, ''); | ||
this._isFiltered = newSearchTerm.length > 2; | ||
this.updateFilteredRows(PreferenceFilterChangeSource.Search); | ||
if (!wasFiltered && this.isFiltered) { | ||
this.expandAll(); |
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.
Makes node expansion the default when searching. At the moment, it only occurs when you switch modes unfiltered -> filtered, but it could be applied on every change of the filter, if that's preferable.
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.
That feels much nicer. I think having the expansion applied on every change of the filter would be a good addition, seems that VSCode does it that way as well
@kenneth-marut-work, thanks for taking a look. I've addressed the issues you identified, I believe. |
This looks good to me 👌 |
1875312
to
f35c308
Compare
Signed-off-by: Colin Grant <colin.grant@ericsson.com>
f35c308
to
ea5d25a
Compare
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.
Some minor comments, looks great!
packages/preferences/src/browser/views/preference-widget-bindings.ts
Outdated
Show resolved
Hide resolved
packages/preferences/src/browser/views/preference-widget-bindings.ts
Outdated
Show resolved
Hide resolved
packages/preferences/src/browser/views/preference-editor-widget.ts
Outdated
Show resolved
Hide resolved
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 confirmed the functionality of the changeset 👍
- the preferences-tree is categorized better, and more organized
- scrolling, expanding, and collapsing the preferences-tree is responsive and quick
- scrolling the preferences-editor correctly updates the preferences-tree
- switching scopes works correctly and is quick
- clicking the
modified in
link works correctly - support for single and multi-root workspaces works correctly
- setting preferences works correctly (string, number, enum values)
-
edit in settings.json
links work correctly - searching for preferences shows the whole path
Signed-off-by: Colin Grant <colin.grant@ericsson.com>
bdcd361
to
0ab4907
Compare
What it does
Fixes: #9138 by adding a 'Commonly Used' section.
Fixes: #9438
Easily visible changes:
Less visible changes
PreferencesEditorWidget
replaced with DOM methods.@injectable()
and able to take advantage of and be instantiated by dependency injection.PreferenceTreeLabelProvider
to allow for dynamic headline generation.DEFAULT_SCROLL_OPTIONS
constant towidget.ts
. In all cases in which a widget uses scroll options in the framework, they use the same settings, but I haven't converted those to references toDEFAULT_SCROLL_OPTIONS
..deepEqual()
method to thePreferenceProvider
class for use inPreferenceProviders
and the node renderers in the preferences package.JSONValue
to the generic on thePreferenceInspection
type, since it always will be aJSONValue
.How to test
Review checklist
Reminder for reviewers
Signed-off-by: Colin Grant colin.grant@ericsson.com