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

Added actions from #57321 to the 'No Settings Found'-message #59739

Merged
merged 3 commits into from Oct 5, 2018

Conversation

Projects
None yet
3 participants
@NiclasvanEyk
Contributor

NiclasvanEyk commented Oct 1, 2018

This PR contains a first suggestion for adding the two actions for clearing all filters and clearing the search to the 'No Settings Found'-message on the settings-search as described in #57321.

The first one is only shown, if there are actually any filters active. The second one is always shown.
I am not sure how to deal with the translations here, since I was not able to find similar, already translated texts I added new ones. If this is not the correct way, please help me here.

As I am new to the codebase I am also not sure, whether the approach with the disposables array is the right one. Is there any documentation about the life cycle of the editor parts available?

@msftclas

This comment has been minimized.

msftclas commented Oct 1, 2018

CLA assistant check
All CLA requirements met.

@roblourens roblourens added this to the October 2018 milestone Oct 1, 2018

@@ -47,6 +47,7 @@ import { CONTEXT_SETTINGS_EDITOR, CONTEXT_SETTINGS_SEARCH_FOCUS, CONTEXT_TOC_ROW
import { IPreferencesService, ISearchResult, ISettingsEditorModel, ISettingsEditorOptions, SettingsEditorOptions, SettingValueType } from 'vs/workbench/services/preferences/common/preferences';
import { SettingsEditor2Input } from 'vs/workbench/services/preferences/common/preferencesEditorInput';
import { Settings2EditorModel } from 'vs/workbench/services/preferences/common/preferencesModels';
import { Disposable } from 'vscode';

This comment has been minimized.

@roblourens

roblourens Oct 1, 2018

Member

Don't import from 'vscode' here, that module is for extensions. You can use IDisposable from lifecycle.ts

This comment has been minimized.

@roblourens

roblourens Oct 1, 2018

Member

Actually you don't need to add that array because SettingsEditor2 already extends Disposable, the _register method adds disposable objects to its internal set of disposables.

This comment has been minimized.

@NiclasvanEyk

NiclasvanEyk Oct 5, 2018

Contributor

Thanks for pointing me in the right direction. I removed the import and replaced the array-logic with calls to _register

@@ -967,6 +1010,11 @@ export class SettingsEditor2 extends BaseEditor {
const parsedQuery = parseQuery(query);
query = parsedQuery.query;
parsedQuery.tags.forEach(tag => this.viewState.tagFilters.add(tag));
// show or hide 'Clear Filters'-link
this.clearFilterLinkContainer.style.display = this.viewState.tagFilters.size > 0

This comment has been minimized.

@roblourens

roblourens Oct 1, 2018

Member

Maybe this should go in renderResultCountMessages?

This comment has been minimized.

@NiclasvanEyk

NiclasvanEyk Oct 5, 2018

Contributor

Yes, this seems like a more appropriate place for this logic. I moved it.

NiclasvanEyk added some commits Oct 5, 2018

made use of _register for disposables
The previous solution involved an own solution with an array of
disposables. The new solution makes use of the _register-method from the
Displosable class.
@roblourens

Looks great, thanks!

@roblourens roblourens merged commit 866c6d6 into Microsoft:master Oct 5, 2018

2 checks passed

VS Code #20181005.26 succeeded with issues
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment