Pick context windows for inlines in a slightly smarter way
ClosedPublic
Actions

Authored by epriestley on May 5 2016, 6:09 PM.

Details

Reviewers

chad

Maniphest Tasks

T10694: Improve behavior of context for inline comments in mail

Commits

rP1baef494c1b7: Pick context windows for inlines in a slightly smarter way

Summary

Ref T10694. This mostly prevents us from having a degenerate case if someone leaves a 200-line inline.

For one-line inlines, show 1 line of context above and below (3 lines total).
For 3+ line inlines, show just the inline.
For 7+ line inlines, show only the first part.

Test Plan

Made a bunch of weird long/short/different-sized comments, saw reasonble-appearing context in text and HTML mail output.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley updated this revision to Diff 38191.May 5 2016, 6:09 PM

epriestley retitled this revision from to Pick context windows for inlines in a slightly smarter way.

epriestley updated this object.

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

epriestley added a reviewer: chad.

epriestley added a task: T10694: Improve behavior of context for inline comments in mail.

chad accepted this revision.May 5 2016, 6:12 PM

chad edited edge metadata.

This revision is now accepted and ready to land.May 5 2016, 6:12 PM

Closed by commit rP1baef494c1b7: Pick context windows for inlines in a slightly smarter way (authored by epriestley, committed by epriestley). · Explain WhyMay 5 2016, 6:15 PM

This revision was automatically updated to reflect the committed changes.

.oO

src/applications/differential/mail/DifferentialInlineCommentMailView.php
289	little inline
297–299	Medium inline.
301–320	BIG INLINE!

what

looks homely

more icons, more roundeder corners, more gradients needed

This is what I'm seeing, which doesn't look terrible:

Screen Shot 2016-05-05 at 11.26.03 AM.png (753×942 px, 114 KB)

Are you seeing something way worse?

oic

I'm not sure we can do icons in mail

But feel free to graident/round everything

Well, doesn't look totally terrible to my untrained engineer-eyes, at least.

let me get kiddo to nap then i'll photomock something

Sounds good.

We can probably use actual unicode character "icons", but it looks like data:.. URIs don't work in a ton of clients and I don't think we can get FontAwesome working in Gmail from googling. We could serve actual images off Phabricator, but I suspect we may be dissatisfied with the results/complexity/support load that generates.

i was joking about icons, I know you love them

Did you see how many icons I added to repositories? Whole thing is basically just different icons now.

epriestley mentioned this in T10694: Improve behavior of context for inline comments in mail.May 5 2016, 6:55 PM

epriestley added inline comments.May 5 2016, 7:11 PM

src/applications/differential/mail/DifferentialInlineCommentMailView.php
297–299	This is a reply with some inline styles.
301–320	This is a reply with a quote block containing a codeblock, which might be probably somewhat difficult to parse visually in mail? HAHAHAHAHA();