Page MenuHomePhabricator

Rough cut of inline comment context

Authored by epriestley on May 5 2016, 11:57 AM.
Referenced Files
Fri, Aug 12, 4:39 AM
Unknown Object (File)
Tue, Aug 9, 8:19 PM
Unknown Object (File)
Sun, Aug 7, 12:40 AM
Unknown Object (File)
Thu, Aug 4, 8:17 AM
Unknown Object (File)
Tue, Aug 2, 10:44 PM
Unknown Object (File)
Sun, Jul 31, 3:39 PM
Unknown Object (File)
Mon, Jul 25, 3:10 PM
Unknown Object (File)
Sat, Jul 23, 5:03 PM



Ref T10694. This is still missing some pieces, but seems to get most of the data into the mail in a plausible format:

  • When an inline remarks on code, show the patch inline in the mail body.
  • When an inline replies to another inline, show that other inline in the mail body.
  • Apply remarkup rendering to inline content.
  • Apply basic styling to mail body blocks.

Not covered yet:

  • Syntax highlighting.
  • Diff highlighting.
  • Maybe clearer style/layout hints to connect comments to what they reply to? Current approach might get messy with inlines that have blockquotes and code blocks inside them, for example.
  • I probably want to cap the amount of diff context we ever show to ~7 lines, even if you drag over 200 lines of code.
  • CSS is a generally a bit rough still.
  • The unified-comment-context option is effectively always on now, and should be removed.
  • Text section is getting indented right now but probably shouldn't be.
  • Spacing, etc., might be a bit off.
Test Plan

Rigged Home to render these things, got a plausible-looking render (top is text, bottom is HTML):

Screen Shot 2016-05-05 at 4.50.18 AM.png (494×329 px, 32 KB)

Sent myself some inline comment mail, got a plausible result.

Diff Detail

rP Phabricator
Lint Not Applicable
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Rough cut of inline comment context.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
chad edited edge metadata.
This revision is now accepted and ready to land.May 5 2016, 4:17 PM
This revision was automatically updated to reflect the committed changes.

test test test


test? test!