Mobile Differential Diff Review (2-up)

Summary:
This does two things

 - Modernizes Table of Contents
 - Makes Differential reasonable on mobile

I say resonable, as you still have to scroll horizontal to see the entire diff. This is minor as the rest of the page is 100x more useful. A 1-up view would be preferred, but this is still an improvement.

Test Plan: Used iOS simulator for browsing diffs.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, chad

Differential Revision: https://secure.phabricator.com/D8681

resources/celerity/map.php

@@ -10,7 +10,7 @@ return array(
     'core.pkg.css' => 'fb144113',
     'core.pkg.js' => 'd3fecc57',
     'darkconsole.pkg.js' => 'ca8671ce',
-    'differential.pkg.css' => '3ad9692c',
+    'differential.pkg.css' => 'cc216438',
     'differential.pkg.js' => '11a5b750',
     'diffusion.pkg.css' => '3783278d',
     'diffusion.pkg.js' => '5b4010f4',
@@ -55,13 +55,13 @@ return array(
     'rsrc/css/application/countdown/timer.css' => '86b7b0a0',
     'rsrc/css/application/diff/inline-comment-summary.css' => '14a91639',
     'rsrc/css/application/differential/add-comment.css' => 'c478bcaa',
-    'rsrc/css/application/differential/changeset-view.css' => 'e710a360',
+    'rsrc/css/application/differential/changeset-view.css' => 'd1951f43',
     'rsrc/css/application/differential/core.css' => '7ac3cabc',
     'rsrc/css/application/differential/results-table.css' => '239924f9',
     'rsrc/css/application/differential/revision-comment.css' => '48186045',
     'rsrc/css/application/differential/revision-history.css' => '0e8eb855',
     'rsrc/css/application/differential/revision-list.css' => 'f3c47d33',
-    'rsrc/css/application/differential/table-of-contents.css' => '19566f76',
+    'rsrc/css/application/differential/table-of-contents.css' => '6bf8e1d2',
     'rsrc/css/application/diffusion/commit-view.css' => '92d1e8f9',
     'rsrc/css/application/diffusion/diffusion-icons.css' => '384a0f7d',
     'rsrc/css/application/diffusion/diffusion-source.css' => '66fdf661',
@@ -505,7 +505,7 @@ return array(
     'conpherence-notification-css' => '403cf598',
     'conpherence-update-css' => '1099a660',
     'conpherence-widget-pane-css' => '87b12e0c',
-    'differential-changeset-view-css' => 'e710a360',
+    'differential-changeset-view-css' => 'd1951f43',
     'differential-core-view-css' => '7ac3cabc',
     'differential-inline-comment-editor' => 'f2441746',
     'differential-results-table-css' => '239924f9',
@@ -513,7 +513,7 @@ return array(
     'differential-revision-comment-css' => '48186045',
     'differential-revision-history-css' => '0e8eb855',
     'differential-revision-list-css' => 'f3c47d33',
-    'differential-table-of-contents-css' => '19566f76',
+    'differential-table-of-contents-css' => '6bf8e1d2',
     'diffusion-commit-view-css' => '92d1e8f9',
     'diffusion-icons-css' => '384a0f7d',
     'diffusion-source-css' => '66fdf661',

src/applications/differential/controller/DifferentialDiffViewController.php

@@ -138,6 +138,7 @@ final class DifferentialDiffViewController extends DifferentialController {
       ),
       array(
         'title' => pht('Diff View'),
+        'device' => true,
       ));
   }
 

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -447,6 +447,7 @@ final class DifferentialRevisionViewController extends DifferentialController {
       array(
         'title' => $object_id.' '.$revision->getTitle(),
         'pageObjects' => array($revision->getPHID()),
+        'device' => true,
       ));
   }
 

src/applications/differential/view/DifferentialDiffTableOfContentsView.php

@@ -122,7 +122,7 @@ final class DifferentialDiffTableOfContentsView extends AphrontView {
 
       $line_count = $changeset->getAffectedLineCount();
       if ($line_count == 0) {
-        $lines = null;
+        $lines = '';
       } else {
         $lines = ' '.pht('(%d line(s))', $line_count);
       }
@@ -135,7 +135,7 @@ final class DifferentialDiffTableOfContentsView extends AphrontView {
       }
       $pchar =
         ($changeset->getOldProperties() === $changeset->getNewProperties())
-          ? null
+          ? ''
           : phutil_tag('span', array('title' => pht('Properties Changed')), 'M')
         ;
 
@@ -150,33 +150,32 @@ final class DifferentialDiffTableOfContentsView extends AphrontView {
             'id' => 'differential-mcoverage-'.md5($fname),
             'class' => 'differential-mcoverage-loading',
           ),
-          (isset($this->visibleChangesets[$id]) ? 'Loading...' : '?'));
+          (isset($this->visibleChangesets[$id]) ?
+            pht('Loading...') : pht('?')));
       }
 
-      $rows[] = phutil_tag('tr', array(), array(
-        phutil_tag(
-          'td',
-          array('class' => 'differential-toc-char', 'title' => $chartitle),
-          $char),
-        phutil_tag('td', array('class' => 'differential-toc-prop'), $pchar),
-        phutil_tag('td', array('class' => 'differential-toc-ftype'), $desc),
-        phutil_tag(
-          'td',
-          array('class' => 'differential-toc-file'),
-          array($link, $lines)),
-        phutil_tag('td', array('class' => 'differential-toc-cov'), $cov),
-        phutil_tag('td', array('class' => 'differential-toc-mcov'), $mcov),
-      ));
       if ($meta) {
-        $rows[] = phutil_tag('tr', array(), array(
+        $meta = phutil_tag(
-          phutil_tag('td', array('colspan' => 3)),
+          'div',
-          phutil_tag('td', array('class' => 'differential-toc-meta'), $meta),
+          array(
-        ));
+            'class' => 'differential-toc-meta'
+          ),
+          $meta);
       }
+
       if ($this->diff && $this->repository) {
         $paths[] =
           $changeset->getAbsoluteRepositoryPath($this->repository, $this->diff);
       }
+
+      $rows[] = array(
+        $char,
+        $pchar,
+        $desc,
+        array($link, $lines, $meta),
+        $cov,
+        $mcov
+      );
     }
 
     $editor_link = null;
@@ -206,37 +205,53 @@ final class DifferentialDiffTableOfContentsView extends AphrontView {
         ),
         pht('Show All Context'));
 
-    $buttons = phutil_tag('tr', array(),
+    $buttons = phutil_tag(
-      phutil_tag('td', array('colspan' => 7),
+      'div',
-        array($editor_link, $reveal_link)));
+      array(
+        'class' => 'differential-toc-buttons grouped'
+      ),
+      array(
+        $editor_link,
+        $reveal_link
+      ));
 
-    $content = hsprintf(
+    $table = id(new AphrontTableView($rows));
-      '%s'.
+    $table->setHeaders(
-      '<div class="differential-toc differential-panel">'.
+      array(
-        '<table>'.
+        '',
-          '<tr>'.
+        '',
-            '<th></th>'.
+        '',
-            '<th></th>'.
+        pht('Path'),
-            '<th></th>'.
-            '<th>Path</th>'.
-            '<th class="differential-toc-cov">%s</th>'.
-            '<th class="differential-toc-mcov">%s</th>'.
-          '</tr>'.
-          '%s%s'.
-        '</table>'.
-      '</div>',
-      id(new PhabricatorAnchorView())
-        ->setAnchorName('toc')
-        ->setNavigationMarker(true)
-        ->render(),
         pht('Coverage (All)'),
         pht('Coverage (Touched)'),
-      phutil_implode_html("\n", $rows),
+      ));
-      $buttons);
+    $table->setColumnClasses(
+      array(
+        'differential-toc-char center',
+        'differential-toc-prop center',
+        'differential-toc-ftype center',
+        'differential-toc-file wide',
+        'differential-toc-cov',
+        'differential-toc-cov',
+      ));
+    $table->setDeviceVisibility(
+      array(
+        true,
+        true,
+        true,
+        true,
+        false,
+        false,
+      ));
+    $anchor = id(new PhabricatorAnchorView())
+        ->setAnchorName('toc')
+        ->setNavigationMarker(true);
 
     return id(new PHUIObjectBoxView())
       ->setHeaderText(pht('Table of Contents'))
-      ->appendChild($content);
+      ->appendChild($anchor)
+      ->appendChild($table)
+      ->appendChild($buttons);
   }
 
   private function renderRename($display_file, $other_file, $arrow) {

webroot/rsrc/css/application/differential/changeset-view.css

@@ -9,6 +9,10 @@
   overflow-x: auto;
 }
 
+.device-phone .differential-changeset {
+  overflow-x: scroll;
+}
+
 .differential-diff {
   background: #fff;
   width: 100%;
@@ -220,6 +224,12 @@ td.cov-X {
 .differential-changeset h1 {
   font-size: 15px;
   padding: 2px 0 12px 12px;
+  line-height: 18px;
+}
+
+.device-phone .differential-changeset h1 {
+  word-break: break-word;
+  margin-right: 8px;
 }
 
 .differential-reticle {
@@ -301,6 +311,11 @@ td.cov-X {
   margin-right: 16px;
 }
 
+.device-phone .differential-changeset-buttons {
+  float: none;
+  margin: 0 0 8px 4px;
+}
+
 .differential-changeset-buttons a.button {
   margin-left: 8px;
 }
@@ -455,3 +470,7 @@ tr.differential-inline-loading {
   margin: 0 4px 2px 0;
   vertical-align: middle;
 }
+
+.device-phone .differential-file-icon-header .phui-icon-view {
+  display: none;
+}

webroot/rsrc/css/application/differential/table-of-contents.css

@@ -3,26 +3,40 @@
  */
 
 .differential-toc-meta {
-  color: {$greytext};
+  color: {$lightgreytext};
-  padding-left: 12px;
+  padding-top: 2px;
 }
 
-.differential-toc-char,
+table.aphront-table-view td.differential-toc-char {
-.differential-toc-prop {
+  padding-right: 0;
-  width: 16px;
-  text-align: center;
   font-weight: bold;
   color: {$darkbluetext};
 }
 
-.differential-toc-ftype {
+table.aphront-table-view td.differential-toc-prop {
-  padding: 0 4px;
+  padding-left: 0;
-  text-align: center;
+  padding-right: 0;
-  color: {$greytext};
+  font-weight: bold;
+  color: {$darkbluetext};
+}
+
+table.aphront-table-view td.differential-toc-ftype {
+  padding-left: 0;
+  font-weight: bold;
+  color: {$darkbluetext};
 }
 
 .differential-toc-file {
-  color: {$greytext};
+  color: {$lightgreytext};
+}
+
+.device-phone .differential-toc-file {
+  word-break: break-word;
+}
+
+.differential-toc-cov {
+  color: {$darkbluetext};
+  font-weight: bold;
 }
 
 .differential-toc-reveal-all,
@@ -36,43 +50,21 @@
 }
 
 .diff-star-okay {
-  color: #ff9700;
+  color: {$orange};
 }
 
 /* TODO: 'warn' and 'fail' are both red, but we can't make 'warn' yellow since
    'okay' is a "gold star". */
 .diff-star-warn {
-  color: #aa0000;
+  color: {$red};
 }
 
 .diff-star-fail {
-  color: #aa0000;
+  color: {$red};
 }
 
 .diff-star-skip {
-  color: #ff00aa;
+  color: {$indigo};
-}
-
-.differential-toc table {
-  width: 100%;
-}
-
-.differential-toc table td.differential-toc-cov,
-.differential-toc table td.differential-toc-mcov {
-  width: 120px;
-  text-align: right;
-  padding-right: 6px;
-}
-
-.differential-toc table th {
-  color: {$darkbluetext};
-  padding: 0 4px 4px 0;
-  white-space: nowrap;
-}
-
-.differential-toc table th.differential-toc-cov,
-.differential-toc table th.differential-toc-mcov {
-  text-align: right;
 }
 
 .differential-toc table td em {
@@ -86,3 +78,8 @@
 .differential-mcoverage-loading {
   color: {$lightgreytext};
 }
+
+.differential-toc-buttons {
+  border-top: 1px solid {$thinblueborder};
+  padding: 8px;
+}