-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[vscode] support CommentThread state property #12454
[vscode] support CommentThread state property #12454
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good, I unfortunately cannot really test due to the menu issues you presented.
@rschnekenbu you might want to enable the ability for maintainers to perform edits so we can update the branch as needed so it satisfies the "out-of-date" check.
packages/plugin-ext/src/main/browser/comments/comment-thread-widget.tsx
Outdated
Show resolved
Hide resolved
@vince-fugnitto , thanks a lot for your review! Still would it make sense to accept the PR? We could add a check on the issue #12452 for this API to be tested through the sample extension. The same should also be done for the already integrated Comment#timestamp property support. And also, unfortunately, I can't switch this ability to perform edit on my own, despite being the creator of the PR. I don't have access to the configuration of the PR. We already tried with Thomas for previous PRs, as he wanted to be able to rebase my PRs on latest main. |
solves eclipse-theia#12231 Contributed on behalf of STMicroelectronics Signed-off-by: Remi Schnekenburger <[email protected]>
7d085e2
to
4dc048f
Compare
I rebased on top of master and I fixed the minor comment. |
@rschnekenbu I think that’s fine, we can revisit and retest this pull-request once the menu is fixed, did I understand that you wanted to fix it? |
@vince-fugnitto, I won't have time to fix the menu issue before next release unfortunately. I already did some investigation, but the refactoring introduced on #11290 was too complex for me to understand the root cause of the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go ahead and keep #12452 in mind to properly review the changes once we fix the regression.
Thanks for the review, @vince-fugnitto! |
What it does
Add Comment#state optional property
This adds the state optional property from Comment to improve vscode api coverage. It also displays the state in the Comment Thread widget
Fixes #12231
Contributed on behalf of ST Microelectronics
How to test
Note: there are currently some issues with menus, since theia 1.28.0. This is due to #11290, integrated in 1.28.0. Prior to this change, the comment sample works on theia 1.27.0. So to test this change, I cherry-picked my commit on top of 1.27.0 tag.
This is the same case as for PR #12007.
The issue #12452 has been created to track those problems about menus.
Review checklist
Reminder for reviewers