From 527dd3ce50b23bd41ebf7d1766886fed75ef2e29 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 17 Mar 2021 12:52:04 -0700 Subject: [PATCH] Replace Differential "unit stars" with icons Summary: Ref T9764. These "star" icons are unclear, inconsistent, and not friendly to colorblind users. They date from a time long ago when the product didn't have icons. Modernize them and make them more consistent with the similar statuses in Harbormaster. Test Plan: {F8545690} {F8545691} {F8545692} Maniphest Tasks: T9764 Differential Revision: https://secure.phabricator.com/D21639 --- src/__phutil_library_map__.php | 4 + .../PhabricatorConfigModuleController.php | 6 +- .../constants/DifferentialConstantsModule.php | 144 ++++++++++++++++++ .../constants/DifferentialUnitStatus.php | 81 ++++++++++ .../customfield/DifferentialUnitField.php | 23 +-- .../DifferentialRevisionUpdateHistoryView.php | 54 ++----- src/view/phui/PHUIColor.php | 13 ++ 7 files changed, 267 insertions(+), 58 deletions(-) create mode 100644 src/applications/differential/constants/DifferentialConstantsModule.php create mode 100644 src/view/phui/PHUIColor.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 5794fac11b..e811ddbc92 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -496,6 +496,7 @@ phutil_register_library_map(array( 'DifferentialCommitsSearchEngineAttachment' => 'applications/differential/engineextension/DifferentialCommitsSearchEngineAttachment.php', 'DifferentialConduitAPIMethod' => 'applications/differential/conduit/DifferentialConduitAPIMethod.php', 'DifferentialConflictsCommitMessageField' => 'applications/differential/field/DifferentialConflictsCommitMessageField.php', + 'DifferentialConstantsModule' => 'applications/differential/constants/DifferentialConstantsModule.php', 'DifferentialController' => 'applications/differential/controller/DifferentialController.php', 'DifferentialCoreCustomField' => 'applications/differential/customfield/DifferentialCoreCustomField.php', 'DifferentialCreateCommentConduitAPIMethod' => 'applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php', @@ -2025,6 +2026,7 @@ phutil_register_library_map(array( 'PHUICalendarMonthView' => 'view/phui/calendar/PHUICalendarMonthView.php', 'PHUICalendarWeekView' => 'view/phui/calendar/PHUICalendarWeekView.php', 'PHUICalendarWidgetView' => 'view/phui/calendar/PHUICalendarWidgetView.php', + 'PHUIColor' => 'view/phui/PHUIColor.php', 'PHUIColorPalletteExample' => 'applications/uiexample/examples/PHUIColorPalletteExample.php', 'PHUICrumbView' => 'view/phui/PHUICrumbView.php', 'PHUICrumbsView' => 'view/phui/PHUICrumbsView.php', @@ -6587,6 +6589,7 @@ phutil_register_library_map(array( 'DifferentialCommitsSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', 'DifferentialConduitAPIMethod' => 'ConduitAPIMethod', 'DifferentialConflictsCommitMessageField' => 'DifferentialCommitMessageField', + 'DifferentialConstantsModule' => 'PhabricatorConfigModule', 'DifferentialController' => 'PhabricatorController', 'DifferentialCoreCustomField' => 'DifferentialCustomField', 'DifferentialCreateCommentConduitAPIMethod' => 'DifferentialConduitAPIMethod', @@ -8356,6 +8359,7 @@ phutil_register_library_map(array( 'PHUICalendarMonthView' => 'AphrontView', 'PHUICalendarWeekView' => 'AphrontView', 'PHUICalendarWidgetView' => 'AphrontTagView', + 'PHUIColor' => 'Phobject', 'PHUIColorPalletteExample' => 'PhabricatorUIExample', 'PHUICrumbView' => 'AphrontView', 'PHUICrumbsView' => 'AphrontView', diff --git a/src/applications/config/controller/module/PhabricatorConfigModuleController.php b/src/applications/config/controller/module/PhabricatorConfigModuleController.php index aaa873f5d2..4f3f9c57cb 100644 --- a/src/applications/config/controller/module/PhabricatorConfigModuleController.php +++ b/src/applications/config/controller/module/PhabricatorConfigModuleController.php @@ -38,7 +38,11 @@ final class PhabricatorConfigModuleController $nav->selectFilter($key); $header = $this->buildHeaderView($title); - $view = $this->buildConfigBoxView($title, $content); + if ($content instanceof AphrontTableView) { + $view = $this->buildConfigBoxView($title, $content); + } else { + $view = $content; + } $crumbs = $this->buildApplicationCrumbs() ->addTextCrumb(pht('Extensions/Modules'), $modules_uri) diff --git a/src/applications/differential/constants/DifferentialConstantsModule.php b/src/applications/differential/constants/DifferentialConstantsModule.php new file mode 100644 index 0000000000..091fa51243 --- /dev/null +++ b/src/applications/differential/constants/DifferentialConstantsModule.php @@ -0,0 +1,144 @@ +getViewer(); + + return array( + $this->renderRevisionStatuses($viewer), + $this->renderUnitStatuses($viewer), + ); + } + + private function renderRevisionStatuses(PhabricatorUser $viewer) { + $statuses = DifferentialRevisionStatus::getAll(); + + $rows = array(); + foreach ($statuses as $status) { + $icon = id(new PHUIIconView()) + ->setIcon( + $status->getIcon(), + $status->getIconColor()); + + $timeline_icon = $status->getTimelineIcon(); + if ($timeline_icon !== null) { + $timeline_view = id(new PHUIIconView()) + ->setIcon( + $status->getTimelineIcon(), + $status->getTimelineColor()); + } else { + $timeline_view = null; + } + + if ($status->isClosedStatus()) { + $is_open = pht('Closed'); + } else { + $is_open = pht('Open'); + } + + $tag_color = $status->getTagColor(); + if ($tag_color !== null) { + $tag_view = id(new PHUIIconView()) + ->seticon('fa-tag', $tag_color); + } else { + $tag_view = null; + } + + $ansi_color = $status->getAnsiColor(); + if ($ansi_color !== null) { + $web_color = PHUIColor::getWebColorFromANSIColor($ansi_color); + $ansi_view = id(new PHUIIconView()) + ->setIcon('fa-stop', $web_color); + } else { + $ansi_view = null; + } + + + $rows[] = array( + $status->getKey(), + $status->getLegacyKey(), + $icon, + $timeline_view, + $tag_view, + $ansi_view, + $is_open, + $status->getDisplayName(), + ); + } + + $table = id(new AphrontTableView($rows)) + ->setHeaders( + array( + pht('Value'), + pht('Legacy Value'), + pht('Icon'), + pht('Timeline Icon'), + pht('Tag Color'), + pht('ANSI Color'), + pht('Open/Closed'), + pht('Name'), + )) + ->setColumnClasses( + array( + null, + null, + null, + null, + null, + null, + null, + 'wide pri', + )); + + $view = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Differential Revision Statuses')) + ->setTable($table); + + return $view; + } + + private function renderUnitStatuses(PhabricatorUser $viewer) { + $statuses = DifferentialUnitStatus::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 Unit Statuses')) + ->setTable($table); + + return $view; + } + +} diff --git a/src/applications/differential/constants/DifferentialUnitStatus.php b/src/applications/differential/constants/DifferentialUnitStatus.php index 2b69853dfb..b8265522fe 100644 --- a/src/applications/differential/constants/DifferentialUnitStatus.php +++ b/src/applications/differential/constants/DifferentialUnitStatus.php @@ -9,4 +9,85 @@ final class DifferentialUnitStatus extends Phobject { const UNIT_SKIP = 4; const UNIT_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->getUnitStatusProperty('name'); + + if ($name === null) { + $name = pht('Unknown Unit Status ("%s")', $this->getValue()); + } + + return $name; + } + + public function getIconIcon() { + return $this->getUnitStatusProperty('icon.icon'); + } + + public function getIconColor() { + return $this->getUnitStatusProperty('icon.color'); + } + + public static function getStatusMap() { + $results = array(); + + foreach (self::newUnitStatusMap() as $value => $ignored) { + $results[$value] = self::newStatusFromValue($value); + } + + return $results; + } + + private function getUnitStatusProperty($key, $default = null) { + $map = self::newUnitStatusMap(); + $properties = idx($map, $this->getValue(), array()); + return idx($properties, $key, $default); + } + + private static function newUnitStatusMap() { + return array( + self::UNIT_NONE => array( + 'name' => pht('No Test Coverage'), + 'icon.icon' => 'fa-ban', + 'icon.color' => 'grey', + ), + self::UNIT_OKAY => array( + 'name' => pht('Tests Passed'), + 'icon.icon' => 'fa-check', + 'icon.color' => 'green', + ), + self::UNIT_WARN => array( + 'name' => pht('Test Warnings'), + 'icon.icon' => 'fa-exclamation-triangle', + 'icon.color' => 'yellow', + ), + self::UNIT_FAIL => array( + 'name' => pht('Test Failures'), + 'icon.icon' => 'fa-times', + 'icon.color' => 'red', + ), + self::UNIT_SKIP => array( + 'name' => pht('Tests Skipped'), + 'icon.icon' => 'fa-fast-forward', + 'icon.color' => 'blue', + ), + self::UNIT_AUTO_SKIP => array( + 'name' => pht('Tests Not Applicable'), + 'icon.icon' => 'fa-upload', + 'icon.color' => 'grey', + ), + ); + } + } diff --git a/src/applications/differential/customfield/DifferentialUnitField.php b/src/applications/differential/customfield/DifferentialUnitField.php index 4c1eb72cc8..6d16b2904d 100644 --- a/src/applications/differential/customfield/DifferentialUnitField.php +++ b/src/applications/differential/customfield/DifferentialUnitField.php @@ -72,29 +72,20 @@ final class DifferentialUnitField } public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + $status_value = $diff->getUnitStatus(); + $status = DifferentialUnitStatus::newStatusFromValue($status_value); - $colors = array( - DifferentialUnitStatus::UNIT_NONE => 'grey', - DifferentialUnitStatus::UNIT_OKAY => 'green', - DifferentialUnitStatus::UNIT_WARN => 'yellow', - DifferentialUnitStatus::UNIT_FAIL => 'red', - DifferentialUnitStatus::UNIT_SKIP => 'blue', - DifferentialUnitStatus::UNIT_AUTO_SKIP => 'blue', - ); - $icon_color = idx($colors, $diff->getUnitStatus(), 'grey'); - - $message = DifferentialRevisionUpdateHistoryView::getDiffUnitMessage( - $diff->getUnitStatus()); + $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)); + ->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 07ca983bc0..6be739fac3 100644 --- a/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php +++ b/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php @@ -153,14 +153,19 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { ), $lint); - $unit = self::renderDiffUnitStar($unit_status); - $unit = phutil_tag( - 'div', - array( - 'class' => 'lintunit-star', - 'title' => self::getDiffUnitMessage($unit_status), - ), - $unit); + $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, + )); $base = $this->renderBaseRevision($diff); } else { @@ -303,20 +308,6 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { return self::renderDiffStar($star); } - private static function renderDiffUnitStar($unit_status) { - static $map = array( - DifferentialUnitStatus::UNIT_NONE => self::STAR_NONE, - DifferentialUnitStatus::UNIT_OKAY => self::STAR_OKAY, - DifferentialUnitStatus::UNIT_WARN => self::STAR_WARN, - DifferentialUnitStatus::UNIT_FAIL => self::STAR_FAIL, - DifferentialUnitStatus::UNIT_SKIP => self::STAR_SKIP, - DifferentialUnitStatus::UNIT_AUTO_SKIP => self::STAR_SKIP, - ); - $star = idx($map, $unit_status, self::STAR_FAIL); - - return self::renderDiffStar($star); - } - public static function getDiffLintMessage(DifferentialDiff $diff) { switch ($diff->getLintStatus()) { case DifferentialLintStatus::LINT_NONE: @@ -335,25 +326,6 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { return pht('Unknown'); } - public static function getDiffUnitMessage($unit_status) { - switch ($unit_status) { - case DifferentialUnitStatus::UNIT_NONE: - return pht('No Unit Test Coverage'); - case DifferentialUnitStatus::UNIT_OKAY: - return pht('Unit Tests OK'); - case DifferentialUnitStatus::UNIT_WARN: - return pht('Unit Test Warnings'); - case DifferentialUnitStatus::UNIT_FAIL: - return pht('Unit Test Errors'); - case DifferentialUnitStatus::UNIT_SKIP: - return pht('Unit Tests Skipped'); - case DifferentialUnitStatus::UNIT_AUTO_SKIP: - return pht( - 'Automatic diff as part of commit; unit tests not applicable.'); - } - return pht('Unknown'); - } - private static function renderDiffStar($star) { $class = 'diff-star-'.$star; return phutil_tag( diff --git a/src/view/phui/PHUIColor.php b/src/view/phui/PHUIColor.php new file mode 100644 index 0000000000..dc96b10914 --- /dev/null +++ b/src/view/phui/PHUIColor.php @@ -0,0 +1,13 @@ + 'sky', + 'magenta' => 'pink', + ); + + return idx($map, $ansi_color, $ansi_color); + } +}