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

Adding Edit/Delete functions to inline comments #1701

Merged
merged 32 commits into from Jun 1, 2018

Conversation

Projects
None yet
6 participants
@StanleyGoldman
Contributor

StanleyGoldman commented May 21, 2018

Depends on:

image

  • Added Edit/Delete comment functionality for inline comment views
    • Even though an administrator can edit/delete any comment. For a first pass we only added the functionality to comments owned by the current viewer.

@StanleyGoldman StanleyGoldman added the WIP label May 21, 2018

StanleyGoldman added some commits May 23, 2018

<StackPanel Orientation="Horizontal" HorizontalAlignment="Right" DockPanel.Dock="Right"
Visibility="{Binding CanEditOrDelete, Converter={ui:BooleanToVisibilityConverter}}">
<ui:GitHubActionLink Content="Edit"

This comment has been minimized.

@jcansdale

jcansdale May 23, 2018

Contributor

image

image

Rather than Edit, should this be Update comment like on .com? 👆

This comment has been minimized.

@donokuda

donokuda May 23, 2018

Member

I think this references the Edit / Delete links in the screenshot above?

Screenshot of feature

If so, I think it's okay to leave this the way how it is. However, I'm 👍 for having the submit button text be "Update comment" like the dotcom screenshot you've posted.

This comment has been minimized.

@StanleyGoldman

StanleyGoldman May 24, 2018

Contributor

Fixed up.

@donokuda

Same feedback as @jcansdale. Otherwise looking good.

</Style.Triggers>
</Style>
</StackPanel.Style>
<Button Command="{Binding CommitEdit}">Edit</Button>

This comment has been minimized.

@donokuda

donokuda May 23, 2018

Member

Per @jcansdale's comment, can we change this to "Update comment" so it's inline with the interaction on dotcom?

This comment has been minimized.

@StanleyGoldman

StanleyGoldman May 24, 2018

Contributor

Fixed up.

StanleyGoldman added some commits May 24, 2018

public ReactiveCommand<ICommentModel> EditComment
{
get { return editComment; }
set

This comment has been minimized.

@grokys

grokys May 29, 2018

Contributor

I see you followed PostComment and made this setter public - I think that was an oversight, it should be protected. When you fix this, could you fix PostComment too?

This comment has been minimized.

@StanleyGoldman

StanleyGoldman May 29, 2018

Contributor

Made them protected

public ReactiveCommand<object> DeleteComment
{
get { return deleteComment; }
set

This comment has been minimized.

@grokys

grokys May 29, 2018

Contributor

As with this one.

This comment has been minimized.

@StanleyGoldman

StanleyGoldman and others added some commits May 29, 2018

Fix to allow diabled octicon buttons
Co-Authored-By: Steven Kirk <grokys@gmail.com>

@meaghanlewis meaghanlewis added this to the 2.5.3 milestone May 29, 2018

@StanleyGoldman

This comment has been minimized.

Contributor

StanleyGoldman commented May 30, 2018

@meaghanlewis good catch. I fixed that up.

@grokys

grokys approved these changes May 31, 2018

x => x.EditState,
x => x == CommentEditState.None && user.Login.Equals(currentUser.Login));
canDelete.Subscribe(b => CanDelete = b);

This comment has been minimized.

@grokys

grokys May 31, 2018

Contributor

You could use ObservableAsPropertyHelper to make this a bit neater.

@grokys

This comment has been minimized.

Contributor

grokys commented May 31, 2018

Oops, clicked "approve" before writing my comment ;)

Looks good and works well - just one small suggestion, but I don't think that should hold up an approval.

@grokys

Oops, spoke too soon. This fails to edit a pending comment:

image

To repro:

  1. Add a comment, click start review
  2. Try to edit the comment

This is because you're using the REST API for editing the comment and the REST API doesn't work with pending comments. We'll have to use GraphQL for this.

@@ -111,7 +111,7 @@ public interface IPullRequestSession
/// <param name="body">The comment body.</param>
/// <param name="inReplyTo">The REST ID of the comment to reply to.</param>
/// <param name="inReplyToNodeId">The GraphQL ID of the comment to reply to.</param>
/// <returns></returns>
/// <returns>A comment model.</returns>

This comment has been minimized.

@sguthals

sguthals May 31, 2018

Contributor

Adding an inline comment

This comment has been minimized.

@StanleyGoldman

StanleyGoldman May 31, 2018

Contributor

Ah. this is a test.

StanleyGoldman added some commits May 31, 2018

@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented May 31, 2018

Nice catch @grokys! I'm able to update comments now when a review is pending.

@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented May 31, 2018

@StanleyGoldman - on a similar note about pending reviews...i don't seem to be able to delete comments while a review is pending. And then trying to update after clicking delete shows a Not Found error even though I still seem to be able to update the comment successfully.

cant delete pending comment

@grokys grokys referenced this pull request May 31, 2018

Merged

Use GraphQL for Pull Request Models. #1712

3 of 3 tasks complete
@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented May 31, 2018

One more thought about this. On dotcom when I go to delete a comment, I get a confirmation popup from the browser:

confirm delete comment

I'm wondering if it would be possible to add a confirmation in Visual Studio as well?

@grokys

grokys approved these changes May 31, 2018

One nit but seems to work fine with pending reviews now!

@@ -24,6 +24,7 @@ public class CommentViewModel : ReactiveObject, ICommentViewModel
CommentEditState state;
DateTimeOffset updatedAt;
string undoBody;
private bool canDelete;

This comment has been minimized.

@grokys

grokys May 31, 2018

Contributor

Nit: Stray private

StanleyGoldman added some commits May 31, 2018

@StanleyGoldman

This comment has been minimized.

Contributor

StanleyGoldman commented May 31, 2018

@meaghanlewis I opened #1713 to reflect your comments

Changes addressed.

@grokys grokys merged commit 0bf5d88 into master Jun 1, 2018

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@grokys grokys deleted the features/inline-comment-edit-and-delete branch Jun 1, 2018

grokys added a commit that referenced this pull request Jun 1, 2018

Merge branch 'master' into refactor/pr-models
Mainly merging #1701

 Conflicts:
	src/GitHub.Exports.Reactive/Services/IPullRequestSession.cs
	src/GitHub.InlineReviews/SampleData/CommentThreadViewModelDesigner.cs
	src/GitHub.InlineReviews/Services/IPullRequestSessionService.cs
	src/GitHub.InlineReviews/Services/PullRequestSession.cs
	src/GitHub.InlineReviews/Services/PullRequestSessionService.cs
	src/GitHub.InlineReviews/ViewModels/CommentThreadViewModel.cs
	src/GitHub.InlineReviews/ViewModels/CommentViewModel.cs
	src/GitHub.InlineReviews/ViewModels/ICommentThreadViewModel.cs
	test/GitHub.InlineReviews.UnitTests/ViewModels/PullRequestReviewCommentViewModelTests.cs

@meaghanlewis meaghanlewis modified the milestones: 2.5.3, 2.5.4 Jun 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment