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

Remove query history title actions #2350

Merged
merged 3 commits into from
Apr 19, 2023

Conversation

norascheuch
Copy link
Contributor

@norascheuch norascheuch commented Apr 18, 2023

Removes the title actions of the query history panel, because newest telemetry show that they are not used by users. Additionally, to unify UI behaviour, we remove them so that we don't have any title actions dependent on a specific selection.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@norascheuch norascheuch requested a review from a team as a code owner April 18, 2023 14:04
robertbrignull
robertbrignull previously approved these changes Apr 18, 2023
Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

🚀

@robertbrignull robertbrignull dismissed their stale review April 18, 2023 14:24

Missed something

@@ -176,9 +176,7 @@ export type QueryHistoryCommands = {
"codeQLQueryHistory.sortByCount": () => Promise<void>;

// Commands in the context menu or in the hover menu
"codeQLQueryHistory.openQueryTitleMenu": TreeViewTitleMultiSelectionCommandFunction<QueryHistoryInfo>;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to remove the TreeViewTitleMultiSelectionCommandFunction type now. The only usage should be for codeQLQueryHistory.itemClicked and that can now have its type changed to TreeViewContextMultiSelectionCommandFunction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I should've checked this.

@charisk
Copy link
Contributor

charisk commented Apr 18, 2023

Thanks for tidying this up. Would be great to update the CHANGELOG too.

Also, do we need to update any docs?

@norascheuch norascheuch force-pushed the nora/remove-query-history-title-bar-actions branch from d3bf91f to 2be0c16 Compare April 19, 2023 08:47
@@ -3,6 +3,7 @@
## [UNRELEASED]

- Add new configuration option to allow downloading databases from http, non-secure servers. [#2332](https://github.com/github/vscode-codeql/pull/2332)
- Remove title actions from the query history panel that depended on history items being selected.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind adding a PR link to match the pattern we use for changelog entries? See the above entry for specifics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Regarding the Docs: we don't seem to mention the titel actions in the docs (https://codeql.github.com/docs/codeql-for-visual-studio-code/analyzing-your-projects/) so I think we don't need to update them.

@robertbrignull
Copy link
Contributor

LGTM once the changelog PR link is added

@norascheuch norascheuch force-pushed the nora/remove-query-history-title-bar-actions branch from 2299b90 to 82c8068 Compare April 19, 2023 08:59
@robertbrignull
Copy link
Contributor

Also, do we need to update any docs?

Docs is an interesting question. I've looked through https://codeql.github.com/docs/codeql-for-visual-studio-code and I can't see any mention of this behaviour, so I think we're good. Even the screenshot at https://codeql.github.com/docs/codeql-for-visual-studio-code/analyzing-your-projects/#viewing-previous-queries doesn't include the title bar actions.

@norascheuch norascheuch merged commit 7f3f338 into main Apr 19, 2023
@norascheuch norascheuch deleted the nora/remove-query-history-title-bar-actions branch April 19, 2023 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants