From 3005b7a7b1146bc12deb3cbfd80d4c8639bd480b Mon Sep 17 00:00:00 2001 From: Chad Little Date: Wed, 2 Apr 2014 21:49:28 -0700 Subject: [PATCH] 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 +- .../DifferentialDiffViewController.php | 1 + .../DifferentialRevisionViewController.php | 1 + .../DifferentialDiffTableOfContentsView.php | 109 ++++++++++-------- .../differential/changeset-view.css | 19 +++ .../differential/table-of-contents.css | 71 ++++++------ 6 files changed, 122 insertions(+), 89 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 547dbc29ac..b1620238b2 100644 --- a/resources/celerity/map.php +++ b/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', diff --git a/src/applications/differential/controller/DifferentialDiffViewController.php b/src/applications/differential/controller/DifferentialDiffViewController.php index dd909da1d6..7ef66f198b 100644 --- a/src/applications/differential/controller/DifferentialDiffViewController.php +++ b/src/applications/differential/controller/DifferentialDiffViewController.php @@ -138,6 +138,7 @@ final class DifferentialDiffViewController extends DifferentialController { ), array( 'title' => pht('Diff View'), + 'device' => true, )); } diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 95f3efa7eb..6906e77ebd 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/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, )); } diff --git a/src/applications/differential/view/DifferentialDiffTableOfContentsView.php b/src/applications/differential/view/DifferentialDiffTableOfContentsView.php index 74391ef5f9..b77d6a4e3d 100644 --- a/src/applications/differential/view/DifferentialDiffTableOfContentsView.php +++ b/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( - phutil_tag('td', array('colspan' => 3)), - phutil_tag('td', array('class' => 'differential-toc-meta'), $meta), - )); + $meta = phutil_tag( + 'div', + 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(), - phutil_tag('td', array('colspan' => 7), - array($editor_link, $reveal_link))); + $buttons = phutil_tag( + 'div', + array( + 'class' => 'differential-toc-buttons grouped' + ), + array( + $editor_link, + $reveal_link + )); - $content = hsprintf( - '%s'. - '
'. - ''. - ''. - ''. - ''. - ''. - ''. - ''. - ''. - ''. - '%s%s'. - '
Path%s%s
'. - '
', - id(new PhabricatorAnchorView()) + $table = id(new AphrontTableView($rows)); + $table->setHeaders( + array( + '', + '', + '', + pht('Path'), + pht('Coverage (All)'), + pht('Coverage (Touched)'), + )); + $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) - ->render(), - pht('Coverage (All)'), - pht('Coverage (Touched)'), - phutil_implode_html("\n", $rows), - $buttons); + ->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) { diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index 69db1cbc8d..a28ad2d3b4 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/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; +} diff --git a/webroot/rsrc/css/application/differential/table-of-contents.css b/webroot/rsrc/css/application/differential/table-of-contents.css index 9a8dce794d..9e8e0a088f 100644 --- a/webroot/rsrc/css/application/differential/table-of-contents.css +++ b/webroot/rsrc/css/application/differential/table-of-contents.css @@ -3,26 +3,40 @@ */ .differential-toc-meta { - color: {$greytext}; - padding-left: 12px; + color: {$lightgreytext}; + padding-top: 2px; } -.differential-toc-char, -.differential-toc-prop { - width: 16px; - text-align: center; +table.aphront-table-view td.differential-toc-char { + padding-right: 0; font-weight: bold; color: {$darkbluetext}; } -.differential-toc-ftype { - padding: 0 4px; - text-align: center; - color: {$greytext}; +table.aphront-table-view td.differential-toc-prop { + padding-left: 0; + padding-right: 0; + 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; -} - -.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; + color: {$indigo}; } .differential-toc table td em { @@ -86,3 +78,8 @@ .differential-mcoverage-loading { color: {$lightgreytext}; } + +.differential-toc-buttons { + border-top: 1px solid {$thinblueborder}; + padding: 8px; +}