Skip to content

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

Merged
shati-patel merged 1 commit intomainfrom
shati-patel/delete-method
Dec 12, 2023
Merged

CodeQL model editor: Make "add" and "delete" buttons more intuitive#3123
shati-patel merged 1 commit intomainfrom
shati-patel/delete-method

Conversation

@shati-patel
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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