From 5954ae84aa7e81c5ad70a8603125033d1650e095 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 31 Jan 2012 12:09:29 -0800 Subject: [PATCH] Improve Herald personal/global UI/UX Summary: - Default "personal" vs "global" choice to "personal". - Don't show global rules under "My Rules". - After editing or creating a global rule, redirect back to global rule list. - Use radio buttons for "personal" vs "global" and add captions explaining the difference. - For "global" rules, don't show the owner/author in the rule detail view -- they effectively have no owner (see also D1387). - For "global" rules, don't show the owner/author in the rule list view, as above. - For admin views, show rule type (global vs personal). Test Plan: - Created and edited new global and personal rules. - Viewed "my", "global" and "admin" views. Reviewers: btrahan, jungejason, nh, xela Reviewed By: btrahan CC: aran, epriestley Differential Revision: https://secure.phabricator.com/D1518 --- src/__celerity_resource_map__.php | 80 +++++++++---------- src/__phutil_library_map__.php | 2 + .../all/HeraldAllRulesController.php | 3 +- .../controller/home/HeraldHomeController.php | 10 ++- .../herald/controller/home/__init__.php | 1 + .../controller/new/HeraldNewController.php | 35 ++++++-- .../herald/controller/new/__init__.php | 1 + .../controller/rule/HeraldRuleController.php | 23 ++++-- .../view/rulelist/HeraldRuleListView.php | 29 ++++++- .../herald/view/rulelist/__init__.php | 1 + .../radio/AphrontFormRadioButtonControl.php | 78 ++++++++++++++++++ src/view/form/control/radio/__init__.php | 15 ++++ webroot/rsrc/css/aphront/form-view.css | 9 +++ 13 files changed, 230 insertions(+), 57 deletions(-) create mode 100644 src/view/form/control/radio/AphrontFormRadioButtonControl.php create mode 100644 src/view/form/control/radio/__init__.php diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 354bb146b9..4dabf8a5a8 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -72,7 +72,7 @@ celerity_register_resource_map(array( ), 'aphront-form-view-css' => array( - 'uri' => '/res/4d1d9d08/rsrc/css/aphront/form-view.css', + 'uri' => '/res/464a73e6/rsrc/css/aphront/form-view.css', 'type' => 'css', 'requires' => array( @@ -1784,6 +1784,30 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/80580cea/differential.pkg.css', 'type' => 'css', ), + '9fd6210b' => + array( + 'name' => 'core.pkg.css', + 'symbols' => + array( + 0 => 'phabricator-core-css', + 1 => 'phabricator-core-buttons-css', + 2 => 'phabricator-standard-page-view', + 3 => 'aphront-dialog-view-css', + 4 => 'aphront-form-view-css', + 5 => 'aphront-panel-view-css', + 6 => 'aphront-side-nav-view-css', + 7 => 'aphront-table-view-css', + 8 => 'aphront-crumbs-view-css', + 9 => 'aphront-tokenizer-control-css', + 10 => 'aphront-typeahead-control-css', + 11 => 'aphront-list-filter-view-css', + 12 => 'phabricator-directory-css', + 13 => 'phabricator-remarkup-css', + 14 => 'syntax-highlighting-css', + ), + 'uri' => '/res/pkg/9fd6210b/core.pkg.css', + 'type' => 'css', + ), 'a6562582' => array( 'name' => 'differential.pkg.js', @@ -1827,43 +1851,19 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/b164acea/javelin.pkg.js', 'type' => 'js', ), - 16378540 => - array( - 'name' => 'core.pkg.css', - 'symbols' => - array( - 0 => 'phabricator-core-css', - 1 => 'phabricator-core-buttons-css', - 2 => 'phabricator-standard-page-view', - 3 => 'aphront-dialog-view-css', - 4 => 'aphront-form-view-css', - 5 => 'aphront-panel-view-css', - 6 => 'aphront-side-nav-view-css', - 7 => 'aphront-table-view-css', - 8 => 'aphront-crumbs-view-css', - 9 => 'aphront-tokenizer-control-css', - 10 => 'aphront-typeahead-control-css', - 11 => 'aphront-list-filter-view-css', - 12 => 'phabricator-directory-css', - 13 => 'phabricator-remarkup-css', - 14 => 'syntax-highlighting-css', - ), - 'uri' => '/res/pkg/16378540/core.pkg.css', - 'type' => 'css', - ), ), 'reverse' => array( - 'aphront-crumbs-view-css' => '16378540', - 'aphront-dialog-view-css' => '16378540', - 'aphront-form-view-css' => '16378540', + 'aphront-crumbs-view-css' => '9fd6210b', + 'aphront-dialog-view-css' => '9fd6210b', + 'aphront-form-view-css' => '9fd6210b', 'aphront-headsup-action-list-view-css' => '80580cea', - 'aphront-list-filter-view-css' => '16378540', - 'aphront-panel-view-css' => '16378540', - 'aphront-side-nav-view-css' => '16378540', - 'aphront-table-view-css' => '16378540', - 'aphront-tokenizer-control-css' => '16378540', - 'aphront-typeahead-control-css' => '16378540', + 'aphront-list-filter-view-css' => '9fd6210b', + 'aphront-panel-view-css' => '9fd6210b', + 'aphront-side-nav-view-css' => '9fd6210b', + 'aphront-table-view-css' => '9fd6210b', + 'aphront-tokenizer-control-css' => '9fd6210b', + 'aphront-typeahead-control-css' => '9fd6210b', 'differential-changeset-view-css' => '80580cea', 'differential-core-view-css' => '80580cea', 'differential-inline-comment-editor' => 'a6562582', @@ -1912,16 +1912,16 @@ celerity_register_resource_map(array( 'javelin-vector' => 'b164acea', 'javelin-workflow' => '46547a92', 'phabricator-content-source-view-css' => '80580cea', - 'phabricator-core-buttons-css' => '16378540', - 'phabricator-core-css' => '16378540', - 'phabricator-directory-css' => '16378540', + 'phabricator-core-buttons-css' => '9fd6210b', + 'phabricator-core-css' => '9fd6210b', + 'phabricator-directory-css' => '9fd6210b', 'phabricator-drag-and-drop-file-upload' => 'a6562582', 'phabricator-keyboard-shortcut' => '46547a92', 'phabricator-keyboard-shortcut-manager' => '46547a92', 'phabricator-object-selector-css' => '80580cea', - 'phabricator-remarkup-css' => '16378540', + 'phabricator-remarkup-css' => '9fd6210b', 'phabricator-shaped-request' => 'a6562582', - 'phabricator-standard-page-view' => '16378540', - 'syntax-highlighting-css' => '16378540', + 'phabricator-standard-page-view' => '9fd6210b', + 'syntax-highlighting-css' => '9fd6210b', ), )); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ec1bd38345..540f58c022 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -38,6 +38,7 @@ phutil_register_library_map(array( 'AphrontFormLayoutView' => 'view/form/layout', 'AphrontFormMarkupControl' => 'view/form/control/markup', 'AphrontFormPasswordControl' => 'view/form/control/password', + 'AphrontFormRadioButtonControl' => 'view/form/control/radio', 'AphrontFormRecaptchaControl' => 'view/form/control/recaptcha', 'AphrontFormSelectControl' => 'view/form/control/select', 'AphrontFormStaticControl' => 'view/form/control/static', @@ -853,6 +854,7 @@ phutil_register_library_map(array( 'AphrontFormLayoutView' => 'AphrontView', 'AphrontFormMarkupControl' => 'AphrontFormControl', 'AphrontFormPasswordControl' => 'AphrontFormControl', + 'AphrontFormRadioButtonControl' => 'AphrontFormControl', 'AphrontFormRecaptchaControl' => 'AphrontFormControl', 'AphrontFormSelectControl' => 'AphrontFormControl', 'AphrontFormStaticControl' => 'AphrontFormControl', diff --git a/src/applications/herald/controller/all/HeraldAllRulesController.php b/src/applications/herald/controller/all/HeraldAllRulesController.php index 2e69d7bb9f..76300b9c65 100644 --- a/src/applications/herald/controller/all/HeraldAllRulesController.php +++ b/src/applications/herald/controller/all/HeraldAllRulesController.php @@ -1,7 +1,7 @@ setRules($rules) ->setHandles($handles) ->setMap($map) + ->setShowType(true) ->setAllowCreation(false) ->setView($this->view); $panel = $list_view->render(); diff --git a/src/applications/herald/controller/home/HeraldHomeController.php b/src/applications/herald/controller/home/HeraldHomeController.php index bb2898d0b4..d0fb2c38ff 100644 --- a/src/applications/herald/controller/home/HeraldHomeController.php +++ b/src/applications/herald/controller/home/HeraldHomeController.php @@ -26,7 +26,7 @@ class HeraldHomeController extends HeraldController { $this->view = idx($data, 'view'); $this->global = idx($data, 'global'); if ($this->global) { - $this->setFilter($this->view . '/global'); + $this->setFilter($this->view.'/global'); } else { $this->setFilter($this->view); } @@ -55,12 +55,13 @@ class HeraldHomeController extends HeraldController { $rules = id(new HeraldRule())->loadAllWhere( 'contentType = %s AND ruleType = %s', $this->view, - 'global'); + HeraldRuleTypeConfig::RULE_TYPE_GLOBAL); } else { $rules = id(new HeraldRule())->loadAllWhere( - 'contentType = %s AND authorPHID = %s', + 'contentType = %s AND authorPHID = %s AND ruleType = %s', $this->view, - $user->getPHID()); + $user->getPHID(), + HeraldRuleTypeConfig::RULE_TYPE_PERSONAL); } foreach ($rules as $rule) { @@ -74,6 +75,7 @@ class HeraldHomeController extends HeraldController { $list_view = id(new HeraldRuleListView()) ->setRules($rules) + ->setShowOwner(!$this->global) ->setHandles($handles) ->setMap($map) ->setAllowCreation(true) diff --git a/src/applications/herald/controller/home/__init__.php b/src/applications/herald/controller/home/__init__.php index a4347e6adb..802301ce76 100644 --- a/src/applications/herald/controller/home/__init__.php +++ b/src/applications/herald/controller/home/__init__.php @@ -7,6 +7,7 @@ phutil_require_module('phabricator', 'applications/herald/config/contenttype'); +phutil_require_module('phabricator', 'applications/herald/config/ruletype'); phutil_require_module('phabricator', 'applications/herald/controller/base'); phutil_require_module('phabricator', 'applications/herald/storage/rule'); phutil_require_module('phabricator', 'applications/herald/view/rulelist'); diff --git a/src/applications/herald/controller/new/HeraldNewController.php b/src/applications/herald/controller/new/HeraldNewController.php index b8df026849..fea5d0a494 100644 --- a/src/applications/herald/controller/new/HeraldNewController.php +++ b/src/applications/herald/controller/new/HeraldNewController.php @@ -41,6 +41,35 @@ class HeraldNewController extends HeraldController { $rule_type_map = HeraldRuleTypeConfig::getRuleTypeMap(); + // Reorder array to put "personal" first. + $rule_type_map = array_select_keys( + $rule_type_map, + array( + HeraldRuleTypeConfig::RULE_TYPE_PERSONAL, + )) + $rule_type_map; + + $captions = array( + HeraldRuleTypeConfig::RULE_TYPE_PERSONAL => + 'Personal rules notify you about events. You own them, but they can '. + 'only affect you.', + HeraldRuleTypeConfig::RULE_TYPE_GLOBAL => + 'Global rules notify anyone about events. No one owns them, and '. + 'anyone can edit them. Usually, Global rules are used to notify '. + 'mailing lists.', + ); + + $radio = id(new AphrontFormRadioButtonControl()) + ->setLabel('Type') + ->setName('rule_type') + ->setValue(HeraldRuleTypeConfig::RULE_TYPE_PERSONAL); + + foreach ($rule_type_map as $value => $name) { + $radio->addButton( + $value, + $name, + idx($captions, $value)); + } + $form = id(new AphrontFormView()) ->setUser($user) ->setAction('/herald/rule/') @@ -50,11 +79,7 @@ class HeraldNewController extends HeraldController { ->setName('content_type') ->setValue($this->contentType) ->setOptions($content_type_map)) - ->appendChild( - id(new AphrontFormSelectControl()) - ->setLabel('Type') - ->setName('rule_type') - ->setOptions($rule_type_map)) + ->appendChild($radio) ->appendChild( id(new AphrontFormSubmitControl()) ->setValue('Create Rule') diff --git a/src/applications/herald/controller/new/__init__.php b/src/applications/herald/controller/new/__init__.php index 8724556b3d..3eb9e2c903 100644 --- a/src/applications/herald/controller/new/__init__.php +++ b/src/applications/herald/controller/new/__init__.php @@ -10,6 +10,7 @@ phutil_require_module('phabricator', 'applications/herald/config/contenttype'); phutil_require_module('phabricator', 'applications/herald/config/ruletype'); phutil_require_module('phabricator', 'applications/herald/controller/base'); phutil_require_module('phabricator', 'view/form/base'); +phutil_require_module('phabricator', 'view/form/control/radio'); phutil_require_module('phabricator', 'view/form/control/select'); phutil_require_module('phabricator', 'view/form/control/submit'); phutil_require_module('phabricator', 'view/layout/panel'); diff --git a/src/applications/herald/controller/rule/HeraldRuleController.php b/src/applications/herald/controller/rule/HeraldRuleController.php index 681965eadc..81e722ef01 100644 --- a/src/applications/herald/controller/rule/HeraldRuleController.php +++ b/src/applications/herald/controller/rule/HeraldRuleController.php @@ -91,6 +91,11 @@ class HeraldRuleController extends HeraldController { if (!$errors) { $uri = '/herald/view/'.$rule->getContentType().'/'; + + if ($rule->getRuleType() == HeraldRuleTypeConfig::RULE_TYPE_GLOBAL) { + $uri .= 'global/'; + } + return id(new AphrontRedirectResponse()) ->setURI($uri); } @@ -134,11 +139,14 @@ class HeraldRuleController extends HeraldController { ->setLabel('Rule Name') ->setName('name') ->setError($e_name) - ->setValue($rule->getName())) - ->appendChild( - id(new AphrontFormMarkupControl()) - ->setLabel('Owner') - ->setValue('
')) + ->setValue($rule->getName())); + + if ($rule->getRuleType() == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) { + $form + ->appendChild( + id(new AphrontFormMarkupControl()) + ->setLabel('Owner') + ->setValue('
')) ->appendChild( // Build this explicitly so we can add a sigil to it. javelin_render_tag( @@ -147,7 +155,10 @@ class HeraldRuleController extends HeraldController { 'type' => 'hidden', 'name' => 'author', 'sigil' => 'author', - ))) + ))); + } + + $form ->appendChild( id(new AphrontFormMarkupControl()) ->setValue( diff --git a/src/applications/herald/view/rulelist/HeraldRuleListView.php b/src/applications/herald/view/rulelist/HeraldRuleListView.php index 49049d801f..ff6c069d87 100644 --- a/src/applications/herald/view/rulelist/HeraldRuleListView.php +++ b/src/applications/herald/view/rulelist/HeraldRuleListView.php @@ -22,6 +22,9 @@ final class HeraldRuleListView extends AphrontView { private $map; private $view; private $allowCreation; + private $showOwner = true; + private $showType = false; + private $user; public function setRules(array $rules) { $this->rules = $rules; @@ -48,12 +51,25 @@ final class HeraldRuleListView extends AphrontView { return $this; } + public function setShowOwner($show_owner) { + $this->showOwner = $show_owner; + return $this; + } + + public function setShowType($show_type) { + $this->showType = $show_type; + return $this; + } + public function setUser($user) { $this->user = $user; return $this; } public function render() { + + $type_map = HeraldRuleTypeConfig::getRuleTypeMap(); + $rows = array(); foreach ($this->rules as $rule) { @@ -93,6 +109,7 @@ final class HeraldRuleListView extends AphrontView { $rows[] = array( $this->map[$rule->getContentType()], + $type_map[$rule->getRuleType()], $owner, $name, $last_edited, @@ -108,7 +125,8 @@ final class HeraldRuleListView extends AphrontView { $table->setHeaders( array( - 'Type', + 'Content Type', + 'Rule Type', 'Owner', 'Rule Name', 'Last Edited', @@ -116,12 +134,21 @@ final class HeraldRuleListView extends AphrontView { )); $table->setColumnClasses( array( + '', '', '', 'wide wrap pri', '', 'action' )); + $table->setColumnVisibility( + array( + true, + $this->showType, + $this->showOwner, + true, + true, + )); $panel = new AphrontPanelView(); $panel->setHeader("Herald Rules for {$rules_for}"); diff --git a/src/applications/herald/view/rulelist/__init__.php b/src/applications/herald/view/rulelist/__init__.php index 9ed50de535..3aa96248cd 100644 --- a/src/applications/herald/view/rulelist/__init__.php +++ b/src/applications/herald/view/rulelist/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'applications/herald/config/ruletype'); phutil_require_module('phabricator', 'infrastructure/javelin/markup'); phutil_require_module('phabricator', 'view/base'); phutil_require_module('phabricator', 'view/control/table'); diff --git a/src/view/form/control/radio/AphrontFormRadioButtonControl.php b/src/view/form/control/radio/AphrontFormRadioButtonControl.php new file mode 100644 index 0000000000..77f5e14aaf --- /dev/null +++ b/src/view/form/control/radio/AphrontFormRadioButtonControl.php @@ -0,0 +1,78 @@ +buttons[] = array( + 'value' => $value, + 'label' => $label, + 'caption' => $caption, + ); + return $this; + } + + protected function getCustomControlClass() { + return 'aphront-form-control-radio'; + } + + protected function renderInput() { + $rows = array(); + foreach ($this->buttons as $button) { + $id = celerity_generate_unique_node_id(); + $radio = phutil_render_tag( + 'input', + array( + 'id' => $id, + 'type' => 'radio', + 'name' => $this->getName(), + 'value' => $button['value'], + 'checked' => ($button['value'] == $this->getValue()) + ? 'checked' + : null, + 'disabled' => $this->getDisabled() ? 'disabled' : null, + )); + $label = phutil_render_tag( + 'label', + array( + 'for' => $id, + ), + phutil_escape_html($button['label'])); + + if (strlen($button['caption'])) { + $label .= + '
'. + phutil_escape_html($button['caption']). + '
'; + } + $rows[] = + ''. + ''.$radio.''. + ''.$label.''. + ''; + } + + return + ''. + implode("\n", $rows). + '
'; + } + +} diff --git a/src/view/form/control/radio/__init__.php b/src/view/form/control/radio/__init__.php new file mode 100644 index 0000000000..e319187eb0 --- /dev/null +++ b/src/view/form/control/radio/__init__.php @@ -0,0 +1,15 @@ +