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

Adding Edit/Delete functions to inline comments #1701

Merged
merged 32 commits into from Jun 1, 2018

Conversation

@StanleyGoldman
Copy link
Contributor

@StanleyGoldman 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.

<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
Collaborator

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
Author Contributor

Fixed up.

Copy link
Member

@donokuda donokuda left a comment

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
Author Contributor

Fixed up.

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
Author 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 May 29, 2018
Author Contributor

👍

@StanleyGoldman StanleyGoldman force-pushed the features/inline-comment-edit-and-delete branch from bd0e9a9 to cc12bea May 29, 2018
@meaghanlewis meaghanlewis added this to the 2.5.3 milestone May 29, 2018
@StanleyGoldman
Copy link
Contributor Author

@StanleyGoldman 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
Copy link
Contributor

@grokys 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.

Copy link
Contributor

@grokys grokys left a comment

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
Author Contributor

Ah. this is a test.

@meaghanlewis
Copy link
Contributor

@meaghanlewis meaghanlewis commented May 31, 2018

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

@meaghanlewis
Copy link
Contributor

@meaghanlewis 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 mentioned this pull request May 31, 2018
3 of 3 tasks complete
@meaghanlewis
Copy link
Contributor

@meaghanlewis 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
Copy link
Contributor

@grokys grokys left a comment

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
Copy link
Contributor Author

@StanleyGoldman StanleyGoldman commented May 31, 2018

@meaghanlewis I opened #1713 to reflect your comments

@grokys grokys dismissed jcansdale’s stale review Jun 1, 2018

Changes addressed.

@grokys grokys merged commit 0bf5d88 into master Jun 1, 2018
2 checks passed
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
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.