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

Ensure results view is opened in column beside #1557

Merged
merged 1 commit into from
Sep 30, 2022

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Sep 29, 2022

The results view will always open next to the current editor.

This restores previous behaviour of the results view. Question: Do we want our other views to have the same behaviour?

Previously, our users specifically asked for the results view to open in a way that would not block the currently active editor.

Fixes #1556

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.

@aeisenberg aeisenberg requested a review from a team as a code owner September 29, 2022 20:03
The results view will always open next to the current editor.
@aeisenberg aeisenberg changed the title Aeisenberg/view column Ensure results view is opened in column beside Sep 29, 2022
@aeisenberg
Copy link
Contributor Author

@adityasharad, this should address the issue you were having earlier with the results view not activating.

@adityasharad
Copy link
Contributor

Yup looks right -- I think b8898b9 correctly ported the results view config but left the column setting hardcoded. I'll let the SecExp team review the change too, in case they have other preferences about window placement or reopening.

@angelapwen
Copy link
Contributor

I tested this out and I'm getting the following error on loading the extension:

Screen Shot 2022-09-29 at 1 15 41 PM

I also no longer see the eval log viewer.

I see the viewer and error on main, though, so perhaps it's to do with forcing this view while it doesn't have anything to load into it yet?

@aeisenberg
Copy link
Contributor Author

Hmm...I can't reproduce that error. Since you're seeing it on main, it's unlikely to be related to this PR.

Can you try doing a clean build of the extension and then launch again?

@angelapwen
Copy link
Contributor

Hmm...I can't reproduce that error. Since you're seeing it on main, it's unlikely to be related to this PR.

Can you try doing a clean build of the extension and then launch again?

Sorry, what I meant was that I wasn't seeing it on main. I'll do the clean build and check again.

@angelapwen
Copy link
Contributor

No longer seeing the error on a clean build 👍

@aeisenberg
Copy link
Contributor Author

As I'm thinking more, most likely this behaviour is not desirable for remote queries since they are run in a different context. Local queries are usually faster and more iterative. The results view for local queries will open automatically after the query completes. This can be annoying if it blocks the tab that you're working on. All other results views appear only when explicitly clicked, so blocking the previously active editor is not a big deal (might actually be desirable).

If you're ok with this, can I get a ✅ ?

Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

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

Reasoning makes sense, 🚢

@aeisenberg aeisenberg merged commit 540d675 into main Sep 30, 2022
@aeisenberg aeisenberg deleted the aeisenberg/view-column branch September 30, 2022 16:22
@aeisenberg
Copy link
Contributor Author

Hmmm...no changelog for this PR. I will add one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Newly opened results view is not opening to the side
3 participants