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

CodeQL model editor: Make "add" and "delete" buttons more intuitive #3123

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

shati-patel
Copy link
Contributor

@shati-patel shati-patel commented Dec 11, 2023

A minor UI suggestion for adding multiple models in the model editor. Previously, you'd click the last row of the model editor to add a new row, and then all other rows would become deletable, except that new empty one. We decided that it's slightly more helpful to have the first row remaining fixed (and undeletable).

Before:
image

After:
image

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
    • I don't think a changelog item is necessary here
  • Issues have been created for any UI or other user-facing changes made by this pull request.
    • See internal linked issue
  • [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.

@shati-patel shati-patel requested a review from a team as a code owner December 11, 2023 16:19
@shati-patel
Copy link
Contributor Author

Some of the Windows tests are failing with

  ● Test suite failed to run

    spawn D:\a\vscode-codeql\vscode-codeql\extensions\ql-vscode\.vscode-test\vscode-win32-x64-archive-1.85.0\Code.exe ENOENT

I'm not quite sure why... It's similar to the errors I've been getting locally though (also on Windows) 🤷🏽

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.

Changes LGTM

I assume the windows test failures are random and not related to this PR. If they keep happening after retrying a few times we can look into whether this PR is special or look into fixing them generally.

Copy link
Contributor

@norascheuch norascheuch left a comment

Choose a reason for hiding this comment

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

I love this change!! I always found it super confusing that the last item wasn't deletable :D (Can you paste a screenshot of the new design to see a before - after?)

@shati-patel
Copy link
Contributor Author

Tests finally passed after a few re-runs 🙃 🎉

(Can you paste a screenshot of the new design to see a before - after?)

Good idea! I've added a before/after to the PR description ❤️

@shati-patel shati-patel merged commit 9cb4d23 into main Dec 12, 2023
14 checks passed
@shati-patel shati-patel deleted the shati-patel/delete-method branch December 12, 2023 10:42
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.

3 participants