Page MenuHomePhabricator

Minor tweaks to pre/inline style for inline comments in HTML mail
ClosedPublic

Authored by epriestley on May 6 2016, 5:54 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Aug 13, 7:31 AM
Unknown Object (File)
Tue, Aug 9, 9:07 AM
Unknown Object (File)
Mon, Aug 8, 2:45 PM
Unknown Object (File)
Sun, Aug 7, 1:46 AM
Unknown Object (File)
Sat, Jul 23, 11:18 AM
Unknown Object (File)
Thu, Jul 21, 3:11 AM
Unknown Object (File)
Jul 13 2022, 3:39 AM
Unknown Object (File)
Jul 12 2022, 7:53 PM
Subscribers
None

Details

Summary

Ref T10694.

  • Shift margins/padding around so inlines with multiple paragraphs get reasonable spacing.
  • Add text-decoration: none to the "View Inline" link to kill the underline.
Test Plan

Screen Shot 2016-05-06 at 10.52.29 AM.png (145×634 px, 25 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Minor tweaks to pre/inline style for inline comments in HTML mail.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
chad edited edge metadata.

can we also add a little space between INLINE COMMENTS and the box? they're very snuggly

This revision is now accepted and ready to land.May 6 2016, 6:00 PM
epriestley edited edge metadata.
  • De-snuggle the list of inlines from the header.
This revision was automatically updated to reflect the committed changes.
src/applications/differential/mail/DifferentialInlineCommentMailView.php
482

I gave the headers a tiny smidgen of extra vertical room too, they felt a little too tight to me compared to the web UI version.

Here is a second paragraph.

This is a third paragraph.

486

I'm adding another comment here to see what the spacing looks like between comments.

What client is that?

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

Now I'm replying to a comment.

This is a second paragraph in my reply to a comment.

This is a code block inside a reply to a comment.

This reply comment also has some more complex comment elements.

Here's a quoted quote.

This one is quite tricky:

Here's a quoted codeblock.

Airmail 2, I don't see it in Polymail or Gmail.

This is what I seen Mail.app, which looks better:

Screen Shot 2016-05-06 at 11.10.41 AM.png (586×871 px, 90 KB)

The initial line of context with no actual content is a little weird, we can probably just drop that. Of course, it's also possible to leave an inline on a completely blank line, so sometimes it's just going to look a bit weird.

It might also be nice to shove all the text to the left (e.g., if every line is indented at least 8 spaces, un-indent every line by 8 spaces), although I think this isn't completely trivial in all cases since sometimes that "whitespace" is really "syntax highlighted whitespace" with tags around it.

You can pay for Litmus if you want, btw. I'll get the CEO to sign off on the expense, just make sure to file all the proper forms and accounting should have it turned around in 5-7 days.

I tried Litmus but it wasn't immediately obvious that it was super useful -- the resolution on Outlook, at least, wasn't good enough for me to make out some of the details like this. Let me see if it reproduces the Airmail 2 issue, though, at least.

I can always fire up the PC too.

I think we should move to NET90 reimbursement, accounting is getting killed on the paper pushers buying all these pencils that they "need".

Well, Litmus does not appear to support Airmail. Let me see if I can guess my way through what's going on and we can pursue a more time-consuming approach if that fails.

Looks Good In Nylas.

Sent from Nylas N1, the extensible, open source
mail client.

It might also be nice to shove all the text to the left (e.g., if every line is indented at least 8 spaces, un-indent every line by 8 spaces), although I think this isn't completely trivial in all cases since sometimes that "whitespace" is really "syntax highlighted whitespace" with tags around it.

I wrote this but it looked really, really weird to me:

Screen Shot 2016-05-06 at 11.39.31 AM.png (121×150 px, 9 KB)

Maybe ok with a rule like "if every line is indented by more than 4 spaces, bring the shallowest indent down to 4 spaces", but I'm just going to drop it for now

For posterity:

// If the entire remaining block is indented by some amount, push it left.
// For instance, if every line is indented by at least 8 spaces, remove the
// first 8 spaces from every line. This makes it easier to read snippets
// on narrow displays like phones.
$common = null;
foreach ($out as $key => $line) {
  $text = $line['text'];
  $length = strlen(rtrim($text));

  // If this line only has whitespace, ignore it.
  if (!$length) {
    continue;
  }

  $leading = 0;
  for ($ii = 0; $ii < $length; $ii++) {
    if ($text[$ii] == ' ') {
      $leading++;
    } else {
      break;
    }
  }

  if ($common === null) {
    $common = $leading;
  } else {
    $common = min($common, $leading);
  }
}

// If we have some common leading whitespace, strip it off every snippet
// line.
if ($common) {
  foreach ($out as $key => $line) {
    $render = (string)$line['render'];
    $render = substr($render, $common);
    $render = phutil_safe_html($render);
    $out[$key]['render'] = $render;
  }
}