Touch up PHP/JS interactions for inline comments
ClosedPublic
Actions

Authored by epriestley on Mar 27 2015, 11:57 PM.

Details

Reviewers

chad

Maniphest Tasks

T1460: Add "Done" link to an inline comment

Commits

Restricted Diffusion Commit
rPa17542ab28c2: Touch up PHP/JS interactions for inline comments

Summary

Ref T1460. Overall:

Pass objectOwnerPHID consistently.
Pass viewer consistently.
Set the correct draft state for checkboxes on the client.

Test Plan

Made inline comments in Differential.
Made inline comments in Diffusion.

Diff Detail

Repository

rP Phabricator

Branch

touchup

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/applications/diffusion/controller/DiffusionBrowseFileController.php:849	XHP16	TODO Comment
Advice	src/applications/diffusion/controller/DiffusionInlineCommentController.php:28	XHP16	TODO Comment

Unit

Tests Passed

Build Status

Buildable 5015
Build 5033: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley updated this revision to Diff 29292.Mar 27 2015, 11:57 PM

epriestley retitled this revision from to Touch up PHP/JS interactions for inline comments.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: chad.

epriestley added a task: T1460: Add "Done" link to an inline comment.

Herald added a subscriber: epriestley. · View Herald TranscriptMar 27 2015, 11:57 PM

epriestley added inline comments.Mar 28 2015, 12:00 AM

src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php
139	I renamed this (currently unused) class because inlines show up on commits too, so "diff author" was a little un-general.
webroot/rsrc/css/application/differential/phui-inline-comment.css
247–250	This is just making sure that the interaction works.

chad accepted this revision.Mar 28 2015, 12:08 AM

chad edited edge metadata.

This revision is now accepted and ready to land.Mar 28 2015, 12:08 AM

Closed by commit rPa17542ab28c2: Touch up PHP/JS interactions for inline comments (authored by epriestley, committed by epriestley). · Explain WhyMar 28 2015, 12:08 AM

This revision was automatically updated to reflect the committed changes.

mmmmm dashed

oh no, it's the evil notchad

Should marking "done" state show in comment previews? It doesn't currently.

It shouldn't be possible to mark "Done" on inline comments which are showing in the inline preview, unless I'm misunderstanding?

There's no preview for the "epriestley marked 17 inlines done." transaction, just because the way that preview is built hasn't been fully modernized. It will show up eventually.

Inlines which you've altered by merely marking "Done" also don't show up in the inline preview. I think this is desirable/correct, at least for now, maybe? Feels kind of heavy/awkward to mix inlines you've added and inlines you've marked "Done" down there. I would expect "epriestley marked 17 inlines done." to show up there eventually, and probably for there to be other queues depending on other views of done-ness.

Generally, my expectation is:

We should show "epriestley marked 17 inlines done." as a transaction preview, and don't only because the code isn't fully modern. This will either autofix eventually or I can explicitly fix it sooner.
Adding the actual comments down there feels kind of heavy/weird offhand, but there's no technical reason we can't pursue it if it feels like it's missing. I'd want to get the transaction preview working first and see if that's good enough, though, and have a clearer idea of where we're going with the top of the revision.