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
2024-11-26 08:42:41 +01:00 · 2012-01-31 12:09:29 -08:00 · 2012-01-31 12:09:29 -08:00 · 5954ae84aa
commit 5954ae84aa
parent 033e4d9997
13 changed files with 230 additions and 57 deletions
--- 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',
  ),
 ));
--- 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',
--- a/src/applications/herald/controller/all/HeraldAllRulesController.php
+++ b/src/applications/herald/controller/all/HeraldAllRulesController.php
@ -1,7 +1,7 @@
 <?php

 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
@ -90,6 +90,7 @@ class HeraldAllRulesController extends HeraldController {
      ->setRules($rules)
      ->setHandles($handles)
      ->setMap($map)
+      ->setShowType(true)
      ->setAllowCreation(false)
      ->setView($this->view);
    $panel = $list_view->render();
--- 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)
--- 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');
--- 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')
--- 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');
--- 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('<div id="author-input"/>'))
+          ->setValue($rule->getName()));
+
+    if ($rule->getRuleType() == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) {
+      $form
+        ->appendChild(
+          id(new AphrontFormMarkupControl())
+            ->setLabel('Owner')
+            ->setValue('<div id="author-input"/>'))
      ->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(
--- 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}");
--- 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');
--- a/src/view/form/control/radio/AphrontFormRadioButtonControl.php
+++ b/src/view/form/control/radio/AphrontFormRadioButtonControl.php
@ -0,0 +1,78 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class AphrontFormRadioButtonControl extends AphrontFormControl {
+
+  private $buttons = array();
+
+  public function addButton($value, $label, $caption) {
+    $this->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 .=
+          '<div class="aphront-form-radio-caption">'.
+            phutil_escape_html($button['caption']).
+          '</div>';
+      }
+      $rows[] =
+        '<tr>'.
+          '<td>'.$radio.'</td>'.
+          '<th>'.$label.'</th>'.
+        '</tr>';
+    }
+
+    return
+      '<table class="aphront-form-control-radio-layout">'.
+        implode("\n", $rows).
+      '</table>';
+  }
+
+}
--- a/src/view/form/control/radio/init.php
+++ b/src/view/form/control/radio/init.php
@ -0,0 +1,15 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'infrastructure/celerity/api');
+phutil_require_module('phabricator', 'view/form/control/base');
+
+phutil_require_module('phutil', 'markup');
+
+
+phutil_require_source('AphrontFormRadioButtonControl.php');
--- a/webroot/rsrc/css/aphront/form-view.css
+++ b/webroot/rsrc/css/aphront/form-view.css
@ -108,22 +108,31 @@
  padding-top: 5px;
 }

+table.aphront-form-control-radio-layout,
 table.aphront-form-control-checkbox-layout {
  margin-top: 3px;
  font-size: 13px;
 }

+table.aphront-form-control-radio-layout th,
 table.aphront-form-control-checkbox-layout th {
  padding-top: 2px;
  padding-left: 0.35em;
  padding-bottom: 4px;
 }

+.aphront-form-control-radio-layout td input,
 .aphront-form-control-checkbox-layout td input {
  margin-top: 4px;
  width: auto;
 }

+.aphront-form-radio-caption {
+  font-size: 11px;
+  color: #444444;
+  max-width: 400px;
+}
+
 .aphront-form-input hr {
  border: none;
  background: #bbbbbb;