Page MenuHomePhabricator

Refine inline style rendering in email
ClosedPublic

Authored by epriestley on May 6 2016, 12:15 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Aug 10, 4:13 AM
Unknown Object (File)
Sat, Aug 6, 12:45 AM
Unknown Object (File)
Fri, Aug 5, 10:27 PM
Unknown Object (File)
Wed, Aug 3, 1:48 PM
Unknown Object (File)
Mon, Aug 1, 3:10 AM
Unknown Object (File)
Sun, Jul 31, 5:46 AM
Unknown Object (File)
Fri, Jul 29, 12:29 PM
Unknown Object (File)
Thu, Jul 28, 1:48 AM
Subscribers
None

Details

Summary

Ref T10694. Move the inline style more toward a mix of standard <pre> style and the web UI style for inlines.

Test Plan

See screenshots in comments.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Refine inline style rendering in email.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.

Here's what this looks like so far when rendered in-browser with slightly more realistic data:

Screen Shot 2016-05-05 at 5.14.18 PM.png (254×1 px, 44 KB)

I think that's better, but doesn't feel nearly as good as your mock.

Let me see if placeholder profile icons just fix everything and then maybe I'll just give in and try to build that out.

(Those would be two different emails, but are a slightly more realistic sequence of events than earlier test data was.)

Here's a with-images version:

Screen Shot 2016-05-05 at 5.33.52 PM.png (304×966 px, 56 KB)

I don't immediately love it so much that I want to go implement email images.

Probably just use two lines:

person1 wrote:
yada yada yada


person2 wrote:
yada yada yada

Also, I'd assume every conversation would have the code in context? Unclear what's going on here:

pasted_file (142×951 px, 45 KB)

The way I've currently implemented this is to drop the code context if you're replying to an inline, under the assumption that the inline you're replying to has enough context on its own and that the use case here is "follow activity from email as a passive observer", not "jump in somewhere random in the middle of the thread and get full context".

That is, every new inline gets exactly one piece of context:

  • If it's a top-level comment, that piece of context is a small diff from the code.
  • If it's a reply, that piece of context is the comment it is replying to.

In particular, I wanted to avoid including the context, one or more replies, and then the new comment, because then we'll be sending email that has much more redundant/old information than new information as soon as threads build up. This could be particularly bad if we choose to include the entire thread and threads get long.

We can revisit this if you think it's a bad direction, I'm just worried that active participants who have been following the thread are going to find email less useful if what used to be one line of new content is now a diff they've already seen, one or more replies they've already read, and then the new comment.

The description in T10694 has more details. Short version is: if you reply to an inline, we currently render no code above your comment in the email, and that's explicitly by design.

But there, the "x" in "x wrote" is always the same user the mail came from, because of the "exactly one piece of context / no code if that context is a comment" behavior.

It feels slightly odd to me to have code context in one inline, but not the other. I didn't pick up on why that was done. I agree that we don't need to replicate the entire conversation. Can we do both, maybe?

code snippet

(n previous comments (link?))
last comment
latest comment

It's fairly straightforward to do that technically, I just worry it's really lowering the signal-to-noise value of the mail.

Over the years, a lot of users have filed requests that amount to "put everything available on the revision page into the mail", and "context + link to thread + parent inline + new inline" feels like a big step in that direction to me. Maybe I just don't really understand this use case, but it would make the mail much less useful to me.

(One possible legitimate use case is "Phabricator is on a VPN, and I can't (or can't easily) VPN from my phone, so I can't click the link", but I've never actually seen a user report this, even though it was something that happened at Facebook for some period of time.)

I generally think you should just click the link if you want to see the revision, and the mail should primarily serve as a notification of what new information is available, so I'm trying to find a compromise here that keeps the focus on new information.

But maybe this is the wrong thing to focus on and we should just dump tons of context into every mail.

In particular, the old feature (from D10146) that I replaced in D15850 basically did this -- it rendered every inline on any line of context in every mail. Here's a simple example of what it looked like:

http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160502/157566.html

This is totally crazy to me. T10283 had a similar screenshot:

inline_comments.png (594×1 px, 123 KB)

These aren't extreme edge cases or anything either with many deeply nested comments or anything -- they're fairly simple cases.

Obviously, we can do a little better than this (at least in HTML mail) but it feels like this pathway leads to ruin to me.

I have some more stuff I want to try here.

epriestley edited edge metadata.
  • Maybe slightly better?

Maybe this is a little better?

Screen Shot 2016-05-06 at 9.06.13 AM.png (269×616 px, 37 KB)

Roughly;

  • Border around the whole thing.
    • Round corners a touch; try to make it look a bit like the actual inline element in the web UI, visually (maybe even use the same colors? Maybe drop the "@" too?).
    • Improve visual clarity/parseability when an inline includes code blocks, quoted blocks, etc.
  • Put a little more whitespace around inline text to pull it out better, visually.
  • Greyer text for quoted text to de-emphasize it a bit and distinguish quoted from new text (maybe too grey? But the next step down looked too black to me).
  • Link directly to the inline.
  • Only show the filename instead of the whole path (I think this is ok 98% of the time and should be better on phones).
  • Tweak colors a little, might be a bit too "epriestley design" now where everything is a grey box though?
chad edited edge metadata.

me gusta

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

I dropped the @ locally since it seems weird/inconsistent the more I look at it.

This probably still won't look quite right in actual mail clients but hopefully I got most of the weirdness fixed.

This revision was automatically updated to reflect the committed changes.
src/applications/differential/mail/DifferentialInlineCommentMailView.php
480

It's also possible we might want slightly more vertical spacing here, or to try the same gold/beige color scheme as the web UI.

Also, maybe nuking the margins/padding on paragraphs completely leaves us with too little space between paragraphs in inline text?

src/applications/differential/mail/DifferentialInlineCommentMailView.php
480

Yeah, I'll tweak the paragraph style a bit. Box margins are also a little off in Mail.app, although most of it doesn't look too bad to me.

The link should also probably have text-decoration: none.