From d6ed9392d4e06a815b95000214ea3c2491b1795b Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 17 Mar 2021 13:36:32 -0700 Subject: [PATCH] Replace Differential "lint stars" with icons Summary: Ref T9764. These stars are inconsistent, not accessible, and generally weird. They predate icons. Update them to use icons instead. Test Plan: {F8545721} {F8545722} {F8545723} Maniphest Tasks: T9764 Differential Revision: https://secure.phabricator.com/D21640 --- resources/celerity/map.php | 6 +- .../constants/DifferentialConstantsModule.php | 36 ++++++ .../constants/DifferentialLintStatus.php | 81 ++++++++++++ .../constants/DifferentialUnitStatus.php | 2 +- .../customfield/DifferentialLintField.php | 30 ++--- .../DifferentialRevisionUpdateHistoryView.php | 115 ++++++------------ .../differential/revision-history.css | 5 - 7 files changed, 168 insertions(+), 107 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index e52ef012ab..3cae51e3f9 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -12,7 +12,7 @@ return array( 'core.pkg.css' => '0ae696de', 'core.pkg.js' => 'ab3502fe', 'dark-console.pkg.js' => '187792c2', - 'differential.pkg.css' => '5c459f92', + 'differential.pkg.css' => 'ffb69e3d', 'differential.pkg.js' => '5080baf4', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '78c9885d', @@ -67,7 +67,7 @@ return array( 'rsrc/css/application/differential/core.css' => '7300a73e', 'rsrc/css/application/differential/phui-inline-comment.css' => '9863a85e', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', - 'rsrc/css/application/differential/revision-history.css' => '8aa3eac5', + 'rsrc/css/application/differential/revision-history.css' => '237a2979', 'rsrc/css/application/differential/revision-list.css' => '93d2df7d', 'rsrc/css/application/differential/table-of-contents.css' => 'bba788b9', 'rsrc/css/application/diffusion/diffusion-icons.css' => '23b31a1b', @@ -569,7 +569,7 @@ return array( 'differential-core-view-css' => '7300a73e', 'differential-revision-add-comment-css' => '7e5900d9', 'differential-revision-comment-css' => '7dbc8d1d', - 'differential-revision-history-css' => '8aa3eac5', + 'differential-revision-history-css' => '237a2979', 'differential-revision-list-css' => '93d2df7d', 'differential-table-of-contents-css' => 'bba788b9', 'diffusion-css' => 'e46232d6', diff --git a/src/applications/differential/constants/DifferentialConstantsModule.php b/src/applications/differential/constants/DifferentialConstantsModule.php index 091fa51243..05a5f06c55 100644 --- a/src/applications/differential/constants/DifferentialConstantsModule.php +++ b/src/applications/differential/constants/DifferentialConstantsModule.php @@ -17,6 +17,7 @@ final class DifferentialConstantsModule return array( $this->renderRevisionStatuses($viewer), $this->renderUnitStatuses($viewer), + $this->renderLintStatuses($viewer), ); } @@ -141,4 +142,39 @@ final class DifferentialConstantsModule return $view; } + private function renderLintStatuses(PhabricatorUser $viewer) { + $statuses = DifferentialLintStatus::getStatusMap(); + + $rows = array(); + foreach ($statuses as $status) { + $rows[] = array( + $status->getValue(), + id(new PHUIIconView()) + ->setIcon($status->getIconIcon(), $status->getIconColor()), + $status->getName(), + ); + } + + $table = id(new AphrontTableView($rows)) + ->setHeaders( + array( + pht('Value'), + pht('Icon'), + pht('Name'), + )) + ->setColumnClasses( + array( + null, + null, + 'wide pri', + )); + + $view = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Differential Lint Statuses')) + ->setTable($table); + + return $view; + } + + } diff --git a/src/applications/differential/constants/DifferentialLintStatus.php b/src/applications/differential/constants/DifferentialLintStatus.php index 72709b8eef..8f2dedbe84 100644 --- a/src/applications/differential/constants/DifferentialLintStatus.php +++ b/src/applications/differential/constants/DifferentialLintStatus.php @@ -9,4 +9,85 @@ final class DifferentialLintStatus extends Phobject { const LINT_SKIP = 4; const LINT_AUTO_SKIP = 6; + private $value; + + public static function newStatusFromValue($value) { + $status = new self(); + $status->value = $value; + return $status; + } + + public function getValue() { + return $this->value; + } + + public function getName() { + $name = $this->getLintStatusProperty('name'); + + if ($name === null) { + $name = pht('Unknown Lint Status ("%s")', $this->getValue()); + } + + return $name; + } + + public function getIconIcon() { + return $this->getLintStatusProperty('icon.icon'); + } + + public function getIconColor() { + return $this->getLintStatusProperty('icon.color'); + } + + public static function getStatusMap() { + $results = array(); + + foreach (self::newLintStatusMap() as $value => $ignored) { + $results[$value] = self::newStatusFromValue($value); + } + + return $results; + } + + private function getLintStatusProperty($key, $default = null) { + $map = self::newLintStatusMap(); + $properties = idx($map, $this->getValue(), array()); + return idx($properties, $key, $default); + } + + private static function newLintStatusMap() { + return array( + self::LINT_NONE => array( + 'name' => pht('No Lint Coverage'), + 'icon.icon' => 'fa-ban', + 'icon.color' => 'grey', + ), + self::LINT_OKAY => array( + 'name' => pht('Lint Passed'), + 'icon.icon' => 'fa-check', + 'icon.color' => 'green', + ), + self::LINT_WARN => array( + 'name' => pht('Lint Warnings'), + 'icon.icon' => 'fa-exclamation-triangle', + 'icon.color' => 'yellow', + ), + self::LINT_FAIL => array( + 'name' => pht('Lint Errors'), + 'icon.icon' => 'fa-times', + 'icon.color' => 'red', + ), + self::LINT_SKIP => array( + 'name' => pht('Lint Skipped'), + 'icon.icon' => 'fa-fast-forward', + 'icon.color' => 'blue', + ), + self::LINT_AUTO_SKIP => array( + 'name' => pht('Lint Not Applicable'), + 'icon.icon' => 'fa-code', + 'icon.color' => 'grey', + ), + ); + } + } diff --git a/src/applications/differential/constants/DifferentialUnitStatus.php b/src/applications/differential/constants/DifferentialUnitStatus.php index b8265522fe..186f476800 100644 --- a/src/applications/differential/constants/DifferentialUnitStatus.php +++ b/src/applications/differential/constants/DifferentialUnitStatus.php @@ -84,7 +84,7 @@ final class DifferentialUnitStatus extends Phobject { ), self::UNIT_AUTO_SKIP => array( 'name' => pht('Tests Not Applicable'), - 'icon.icon' => 'fa-upload', + 'icon.icon' => 'fa-code', 'icon.color' => 'grey', ), ); diff --git a/src/applications/differential/customfield/DifferentialLintField.php b/src/applications/differential/customfield/DifferentialLintField.php index 62b94c51eb..faf065a8d7 100644 --- a/src/applications/differential/customfield/DifferentialLintField.php +++ b/src/applications/differential/customfield/DifferentialLintField.php @@ -38,7 +38,6 @@ final class DifferentialLintField protected function getDiffPropertyKeys() { return array( 'arc:lint', - 'arc:lint-excuse', ); } @@ -84,33 +83,18 @@ final class DifferentialLintField DifferentialDiff $diff, array $messages) { - $colors = array( - DifferentialLintStatus::LINT_NONE => 'grey', - DifferentialLintStatus::LINT_OKAY => 'green', - DifferentialLintStatus::LINT_WARN => 'yellow', - DifferentialLintStatus::LINT_FAIL => 'red', - DifferentialLintStatus::LINT_SKIP => 'blue', - DifferentialLintStatus::LINT_AUTO_SKIP => 'blue', - ); - $icon_color = idx($colors, $diff->getLintStatus(), 'grey'); + $status_value = $diff->getLintStatus(); + $status = DifferentialLintStatus::newStatusFromValue($status_value); - $message = DifferentialRevisionUpdateHistoryView::getDiffLintMessage($diff); - - $excuse = $diff->getProperty('arc:lint-excuse'); - if (strlen($excuse)) { - $excuse = array( - phutil_tag('strong', array(), pht('Excuse:')), - ' ', - phutil_escape_html_newlines($excuse), - ); - } + $status_icon = $status->getIconIcon(); + $status_color = $status->getIconColor(); + $status_name = $status->getName(); $status = id(new PHUIStatusListView()) ->addItem( id(new PHUIStatusItemView()) - ->setIcon(PHUIStatusItemView::ICON_STAR, $icon_color) - ->setTarget($message) - ->setNote($excuse)); + ->setIcon($status_icon, $status_color) + ->setTarget($status_name)); return $status; } diff --git a/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php b/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php index 6be739fac3..2e074a2f7e 100644 --- a/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php +++ b/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php @@ -139,34 +139,8 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { } if ($diff) { - $unit_status = idx( - $this->unitStatus, - $diff->getPHID(), - $diff->getUnitStatus()); - - $lint = self::renderDiffLintStar($row['obj']); - $lint = phutil_tag( - 'div', - array( - 'class' => 'lintunit-star', - 'title' => self::getDiffLintMessage($diff), - ), - $lint); - - $status = DifferentialUnitStatus::newStatusFromValue($unit_status); - - $unit_icon = $status->getIconIcon(); - $unit_color = $status->getIconColor(); - $unit_name = $status->getName(); - - $unit = id(new PHUIIconView()) - ->setIcon($unit_icon, $unit_color) - ->addSigil('has-tooltip') - ->setMetadata( - array( - 'tip' => $unit_name, - )); - + $lint = $this->newLintStatusView($diff); + $unit = $this->newUnitStatusView($diff); $base = $this->renderBaseRevision($diff); } else { $lint = null; @@ -287,53 +261,6 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { return $content; } - const STAR_NONE = 'none'; - const STAR_OKAY = 'okay'; - const STAR_WARN = 'warn'; - const STAR_FAIL = 'fail'; - const STAR_SKIP = 'skip'; - - private static function renderDiffLintStar(DifferentialDiff $diff) { - static $map = array( - DifferentialLintStatus::LINT_NONE => self::STAR_NONE, - DifferentialLintStatus::LINT_OKAY => self::STAR_OKAY, - DifferentialLintStatus::LINT_WARN => self::STAR_WARN, - DifferentialLintStatus::LINT_FAIL => self::STAR_FAIL, - DifferentialLintStatus::LINT_SKIP => self::STAR_SKIP, - DifferentialLintStatus::LINT_AUTO_SKIP => self::STAR_SKIP, - ); - - $star = idx($map, $diff->getLintStatus(), self::STAR_FAIL); - - return self::renderDiffStar($star); - } - - public static function getDiffLintMessage(DifferentialDiff $diff) { - switch ($diff->getLintStatus()) { - case DifferentialLintStatus::LINT_NONE: - return pht('No Linters Available'); - case DifferentialLintStatus::LINT_OKAY: - return pht('Lint OK'); - case DifferentialLintStatus::LINT_WARN: - return pht('Lint Warnings'); - case DifferentialLintStatus::LINT_FAIL: - return pht('Lint Errors'); - case DifferentialLintStatus::LINT_SKIP: - return pht('Lint Skipped'); - case DifferentialLintStatus::LINT_AUTO_SKIP: - return pht('Automatic diff as part of commit; lint not applicable.'); - } - return pht('Unknown'); - } - - private static function renderDiffStar($star) { - $class = 'diff-star-'.$star; - return phutil_tag( - 'span', - array('class' => $class), - "\xE2\x98\x85"); - } - private function renderBaseRevision(DifferentialDiff $diff) { switch ($diff->getSourceControlSystem()) { case 'git': @@ -373,4 +300,42 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { } return $link; } + + private function newLintStatusView(DifferentialDiff $diff) { + $value = $diff->getLintStatus(); + $status = DifferentialLintStatus::newStatusFromValue($value); + + $icon = $status->getIconIcon(); + $color = $status->getIconColor(); + $name = $status->getName(); + + return $this->newDiffStatusIconView($icon, $color, $name); + } + + private function newUnitStatusView(DifferentialDiff $diff) { + $value = $diff->getUnitStatus(); + + // NOTE: We may be overriding the value on the diff with a value from + // Harbormaster. + $value = idx($this->unitStatus, $diff->getPHID(), $value); + + $status = DifferentialUnitStatus::newStatusFromValue($value); + + $icon = $status->getIconIcon(); + $color = $status->getIconColor(); + $name = $status->getName(); + + return $this->newDiffStatusIconView($icon, $color, $name); + } + + private function newDiffStatusIconView($icon, $color, $name) { + return id(new PHUIIconView()) + ->setIcon($icon, $color) + ->addSigil('has-tooltip') + ->setMetadata( + array( + 'tip' => $name, + )); + } + } diff --git a/webroot/rsrc/css/application/differential/revision-history.css b/webroot/rsrc/css/application/differential/revision-history.css index eca1a95ceb..0a2cc3b381 100644 --- a/webroot/rsrc/css/application/differential/revision-history.css +++ b/webroot/rsrc/css/application/differential/revision-history.css @@ -54,8 +54,3 @@ td.differential-update-history-new { background: #aaffaa; } - -.lintunit-star { - text-align: center; - padding: 0 16px; -}