From 35d03d36c72a35a2db773532635d51f3b94fcd91 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 31 Jul 2011 08:38:06 -0700 Subject: [PATCH] Improve display behavior of commit messages in Diffusion Summary: See T372. Always render commit messages on one display line, so the table doesn't jump around as they AJAX in on browse views. The goal here is to have the cell choose a size naturally and for its content to render with "overflow: hidden" if the natural size isn't large enough to contain the content. "white-space: pre" or "white-space: nowrap" would prevent wrapping but potentially make the table exceed the display width when a better behavior is to hide some of the commit message. Also use utf8-aware shortening, now that we have a function for it. Casting a wide net in case anyone has a better way to do the CSS here. It's kind of nasty that we have to use so many DOM nodes. Test Plan: - Resized window while viewing browse and history views in Safari, Chrome and Firefox. Table exhibited described behavior. - Verified summaries render sensibly and are properly truncated to 100 characters. Reviewed By: aran Reviewers: aran, jungejason, tuomaspelkonen, tomo, mroch, cpojer CC: aran, epriestley Differential Revision: 750 --- .../browsetable/DiffusionBrowseTableView.php | 3 ++- .../DiffusionHistoryTableView.php | 5 ++-- .../PhabricatorRepositoryCommitData.php | 3 ++- src/view/control/table/AphrontTableView.php | 24 +++++++++++++++++++ src/view/control/table/__init__.php | 1 + webroot/rsrc/css/aphront/table-view.css | 12 ++++++++++ 6 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/applications/diffusion/view/browsetable/DiffusionBrowseTableView.php b/src/applications/diffusion/view/browsetable/DiffusionBrowseTableView.php index 66a0b14ed5..84bddf952b 100644 --- a/src/applications/diffusion/view/browsetable/DiffusionBrowseTableView.php +++ b/src/applications/diffusion/view/browsetable/DiffusionBrowseTableView.php @@ -57,7 +57,8 @@ final class DiffusionBrowseTableView extends DiffusionView { } else { $author = phutil_escape_html($data->getAuthorName()); } - $details = phutil_escape_html($data->getSummary()); + $details = AphrontTableView::renderSingleDisplayLine( + phutil_escape_html($data->getSummary())); } else { $author = ''; $details = ''; diff --git a/src/applications/diffusion/view/historytable/DiffusionHistoryTableView.php b/src/applications/diffusion/view/historytable/DiffusionHistoryTableView.php index 283cf99828..8012a98d28 100644 --- a/src/applications/diffusion/view/historytable/DiffusionHistoryTableView.php +++ b/src/applications/diffusion/view/historytable/DiffusionHistoryTableView.php @@ -77,7 +77,8 @@ final class DiffusionHistoryTableView extends DiffusionView { $date, $time, $author, - phutil_escape_html($history->getSummary()), + AphrontTableView::renderSingleDisplayLine( + phutil_escape_html($history->getSummary())), // TODO: etc etc ); } @@ -101,7 +102,7 @@ final class DiffusionHistoryTableView extends DiffusionView { '', 'right', '', - 'wide wrap', + 'wide', )); return $view->render(); } diff --git a/src/applications/repository/storage/commitdata/PhabricatorRepositoryCommitData.php b/src/applications/repository/storage/commitdata/PhabricatorRepositoryCommitData.php index 9c6825d0ac..5f2abaff21 100644 --- a/src/applications/repository/storage/commitdata/PhabricatorRepositoryCommitData.php +++ b/src/applications/repository/storage/commitdata/PhabricatorRepositoryCommitData.php @@ -38,7 +38,8 @@ class PhabricatorRepositoryCommitData extends PhabricatorRepositoryDAO { $message = $this->getCommitMessage(); $lines = explode("\n", $message); $summary = head($lines); - $summary = substr($summary, 0, self::SUMMARY_MAX_LENGTH); + + $summary = phutil_utf8_shorten($summary, self::SUMMARY_MAX_LENGTH); return $summary; } diff --git a/src/view/control/table/AphrontTableView.php b/src/view/control/table/AphrontTableView.php index da7fdb50eb..8e4de285c5 100644 --- a/src/view/control/table/AphrontTableView.php +++ b/src/view/control/table/AphrontTableView.php @@ -154,5 +154,29 @@ class AphrontTableView extends AphrontView { $table[] = ''; return implode('', $table); } + + public static function renderSingleDisplayLine($line) { + + // TODO: Is there a cleaner way to do this? We use a relative div with + // overflow hidden to provide the bounds, and an absolute span with + // white-space: pre to prevent wrapping. We need to append a character + // (  -- nonbreaking space) afterward to give the bounds div height + // (alternatively, we could hard-code the line height). This is gross but + // it's not clear that there's a better appraoch. + + return phutil_render_tag( + 'div', + array( + 'class' => 'single-display-line-bounds', + ), + phutil_render_tag( + 'span', + array( + 'class' => 'single-display-line-content', + ), + $line).' '); + } + + } diff --git a/src/view/control/table/__init__.php b/src/view/control/table/__init__.php index c739a74a41..b530a98f17 100644 --- a/src/view/control/table/__init__.php +++ b/src/view/control/table/__init__.php @@ -9,6 +9,7 @@ phutil_require_module('phabricator', 'infrastructure/celerity/api'); phutil_require_module('phabricator', 'view/base'); +phutil_require_module('phutil', 'markup'); phutil_require_module('phutil', 'utils'); diff --git a/webroot/rsrc/css/aphront/table-view.css b/webroot/rsrc/css/aphront/table-view.css index 7d7e58557b..d428a8af22 100644 --- a/webroot/rsrc/css/aphront/table-view.css +++ b/webroot/rsrc/css/aphront/table-view.css @@ -74,6 +74,17 @@ white-space: normal; } +div.single-display-line-bounds { + width: 100%; + position: relative; + overflow: hidden; +} + +span.single-display-line-content { + white-space: pre; + position: absolute; +} + .aphront-table-view tr.highlighted { background: #ffff99; } @@ -94,3 +105,4 @@ max-width: 64px; max-height: 64px; } +