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

Add action to focus query editor widget #94799

Merged
merged 8 commits into from
Apr 10, 2020

Conversation

kdnk
Copy link
Contributor

@kdnk kdnk commented Apr 9, 2020

This PR fixes #94439

@msftclas
Copy link

msftclas commented Apr 9, 2020

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@JacksonKearl JacksonKearl left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor changes.

@@ -207,6 +207,9 @@ registry.registerWorkbenchAction(
registry.registerWorkbenchAction(SyncActionDescriptor.create(RerunSearchEditorSearchAction, RerunSearchEditorSearchAction.ID, RerunSearchEditorSearchAction.LABEL,
{ mac: { primary: KeyMod.CtrlCmd | KeyMod.Shift | KeyCode.KEY_R } }, ContextKeyExpr.and(SearchEditorConstants.InSearchEditor)),
'Search Editor: Search Again', category);

registry.registerWorkbenchAction(SyncActionDescriptor.create(FocusQueryEditorWidgetAction, FocusQueryEditorWidgetAction.ID, FocusQueryEditorWidgetAction.LABEL),
'Search Editor: Focus Query Editor Widget', category);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd default this to be bound to Escape, then we can remove the existing line binding it to Escape in searchEditor.ts and instead use only this. That way people that want to use Escape for other things aren't conflicting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if (viewState && viewState.focused === 'editor') {
this.queryEditorWidget.searchInput.focus();
} else {
this.searchResultEditor.focus();
Copy link
Contributor

Choose a reason for hiding this comment

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

No real need to make this toggle, it can simply call this.queryEditorWidget.searchInput.focus();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JacksonKearl JacksonKearl added this to the April 2020 milestone Apr 9, 2020
@kdnk kdnk changed the title WIP: Focus query editor widget Focus query editor widget Apr 10, 2020
@kdnk kdnk changed the title Focus query editor widget Add action to focus query editor widget Apr 10, 2020
@kdnk kdnk requested a review from JacksonKearl April 10, 2020 13:18
@@ -207,6 +207,10 @@ registry.registerWorkbenchAction(
registry.registerWorkbenchAction(SyncActionDescriptor.create(RerunSearchEditorSearchAction, RerunSearchEditorSearchAction.ID, RerunSearchEditorSearchAction.LABEL,
{ mac: { primary: KeyMod.CtrlCmd | KeyMod.Shift | KeyCode.KEY_R } }, ContextKeyExpr.and(SearchEditorConstants.InSearchEditor)),
'Search Editor: Search Again', category);

registry.registerWorkbenchAction(SyncActionDescriptor.create(FocusQueryEditorWidgetAction, FocusQueryEditorWidgetAction.ID, FocusQueryEditorWidgetAction.LABEL,
{ mac: { primary: KeyCode.Escape } }, ContextKeyExpr.and(SearchEditorConstants.InSearchEditor)),
Copy link
Contributor

Choose a reason for hiding this comment

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

This can just be { primary: KeyCode.Escape }, rather than being mac specific. Some of the other bindings are macOS specific because Windows binds them to system commands, but that isn't the case here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙈
Fixed fb4b628

@kdnk kdnk requested a review from JacksonKearl April 10, 2020 23:27
@JacksonKearl
Copy link
Contributor

Looks good, thanks!

@JacksonKearl JacksonKearl merged commit be3df0a into microsoft:master Apr 10, 2020
@kdnk kdnk deleted the focusQueryEditorWidget branch April 11, 2020 00:41
@github-actions github-actions bot locked and limited conversation to collaborators May 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SearchEditor: Add command to focus queryEditorWidget
3 participants