MAGETWO-89659: Address comments / feedback from review

Robert He · Robert He · commit 6b64e128a193 · 2018-04-03T17:17:56.000-05:00
- fix bugs and stylings
diff --git a/app/code/Magento/Ui/Component/Form/Element/ColorPicker.php b/app/code/Magento/Ui/Component/Form/Element/ColorPicker.php
@@ -14,6 +14,8 @@
 use Magento\Framework\View\Element\UiComponent\ContextInterface;
 
 /**
+ * Class ColorPicker
+ *
  * @api
  */
 class ColorPicker extends AbstractElement
diff --git a/app/code/Magento/Ui/Model/ColorPicker/ColorModesProvider.php b/app/code/Magento/Ui/Model/ColorPicker/ColorModesProvider.php
@@ -8,6 +8,9 @@
 
 namespace Magento\Ui\Model\ColorPicker;
 
+/**
+ * Class ColorModesProvider
+ */
 class ColorModesProvider
 {
     private $colorModes;
@@ -22,7 +25,12 @@ public function __construct(
         $this->objectManager = $objectManager;
     }
 
-    public function getModes()
+    /**
+     * Return all available modes and their configuration
+     *
+     * @return array
+     */
+    public function getModes(): array
     {
         $config = [];
         foreach ($this->colorModes as $modeName => $className) {
@@ -36,9 +44,9 @@ public function getModes()
      * Create mode provider
      *
      * @param string $instance
-     * @return ConfigInterface
+     * @return ModeInterface
      */
-    private function createModeProvider($instance)
+    private function createModeProvider(string $instance): ModeInterface
     {
         if (!is_subclass_of(
             $instance,
diff --git a/app/code/Magento/Ui/Model/ColorPicker/FullMode.php b/app/code/Magento/Ui/Model/ColorPicker/FullMode.php
@@ -18,7 +18,7 @@ class FullMode implements ModeInterface
      *
      * @return array
      */
-    public function getConfig()
+    public function getConfig(): array
     {
         return [
             'showInput' => true,
diff --git a/app/code/Magento/Ui/Model/ColorPicker/NoAlphaMode.php b/app/code/Magento/Ui/Model/ColorPicker/NoAlphaMode.php
@@ -9,7 +9,7 @@
 namespace Magento\Ui\Model\ColorPicker;
 
 /**
- * Returns config parameters for full mode
+ * Returns config parameters for noalpha mode
  */
 class NoAlphaMode implements ModeInterface
 {
@@ -18,7 +18,7 @@ class NoAlphaMode implements ModeInterface
      *
      * @return array
      */
-    public function getConfig()
+    public function getConfig(): array
     {
         return [
             'showInput' => true,
diff --git a/app/code/Magento/Ui/Model/ColorPicker/PaletteOnlyMode.php b/app/code/Magento/Ui/Model/ColorPicker/PaletteOnlyMode.php
@@ -9,7 +9,7 @@
 namespace Magento\Ui\Model\ColorPicker;
 
 /**
- * Returns config parameters for full mode
+ * Returns config parameters for paletteonly mode
  */
 class PaletteOnlyMode implements ModeInterface
 {
@@ -18,14 +18,15 @@ class PaletteOnlyMode implements ModeInterface
      *
      * @return array
      */
-    public function getConfig()
+    public function getConfig(): array
     {
         return [
             'showInput' => false,
             'showInitial' => false,
             'showPalette' => true,
+            'showPaletteOnly' => true,
             'showAlpha' => false,
-            'showSelectionPalette' => true
+            'showSelectionPalette' => false
         ];
     }
 }
diff --git a/app/code/Magento/Ui/Model/ColorPicker/SimpleMode.php b/app/code/Magento/Ui/Model/ColorPicker/SimpleMode.php
@@ -9,7 +9,7 @@
 namespace Magento\Ui\Model\ColorPicker;
 
 /**
- * Returns config parameters for full mode
+ * Returns config parameters for simple mode
  */
 class SimpleMode implements ModeInterface
 {
@@ -18,7 +18,7 @@ class SimpleMode implements ModeInterface
      *
      * @return array
      */
-    public function getConfig()
+    public function getConfig(): array
     {
         return [
             'showInput' => false,
diff --git a/app/code/Magento/Ui/etc/ui_configuration.xsd b/app/code/Magento/Ui/etc/ui_configuration.xsd
@@ -455,8 +455,8 @@
     <xs:element name="colorPicker" type="componentColorPicker">
         <xs:annotation>
             <xs:documentation>
-                The ColorPicker component utilizes the Spectrum js library to allow user greater flexibility in entering
-                color values.
+                The ColorPicker component uses the Spectrum and tinycolor .js libraries to make it easier to choose and
+                implement color values.
             </xs:documentation>
         </xs:annotation>
     </xs:element>
diff --git a/app/code/Magento/Ui/view/base/ui_component/etc/definition.xml b/app/code/Magento/Ui/view/base/ui_component/etc/definition.xml
@@ -149,7 +149,7 @@
             <initialMediaGalleryOpenSubpath>wysiwyg</initialMediaGalleryOpenSubpath>
         </settings>
     </imageUploader>
-    <colorPicker class="Magento\Ui\Component\Form\Element\ColorPicker" component="Magento_Ui/js/form/element/colorPicker" template="ui/form/field">
+    <colorPicker class="Magento\Ui\Component\Form\Element\ColorPicker" component="Magento_Ui/js/form/element/color-picker" template="ui/form/field">
         <settings>
             <elementTmpl>ui/form/element/colorPicker</elementTmpl>
             <colorFormat>rgb</colorFormat>
diff --git a/app/code/Magento/Ui/view/base/web/js/form/element/color-picker.js b/app/code/Magento/Ui/view/base/web/js/form/element/color-picker.js
@@ -1,15 +1,15 @@
+/**
+ * Copyright © Magento, Inc. All rights reserved.
+ * See COPYING.txt for license details.
+ */
 
 /**
  * @api
  */
 define([
-    'jquery',
-    'underscore',
-    'uiLayout',
     'mage/translate',
-    'Magento_Ui/js/form/element/abstract',
-    'spectrum'
-], function ($, _, layout, $t, Abstract, spectrum) {
+    'Magento_Ui/js/form/element/abstract'
+], function ($t, Abstract) {
     'use strict';
 
     var defaultColorPalette = [
@@ -43,17 +43,10 @@ define([
          */
         initialize: function () {
             this._super();
+
             this.colorPickerConfig.value = this.value;
-            // $(document).on('click', '.sp-replacer', function () {
-            //     this.focused(true);
-            // });
             return this;
-        },
-
-        // setFocus: function () {
-        //     this.focused(true);
-        // }
-
+        }
     });
 });
  
diff --git a/app/code/Magento/Ui/view/base/web/js/lib/knockout/bindings/colorPicker.js b/app/code/Magento/Ui/view/base/web/js/lib/knockout/bindings/colorPicker.js
@@ -6,8 +6,9 @@ define([
     'ko',
     'jquery',
     '../template/renderer',
-    'spectrum'
-], function (ko, $, renderer, spectrum) {
+    'spectrum',
+    'tinycolor'
+], function (ko, $, renderer, spectrum, tinycolor) {
     'use strict';
 
     ko.bindingHandlers.colorPicker = {
@@ -22,24 +23,40 @@ define([
          */
         init: function (element, valueAccessor, allBindings, viewModel) {
             var config = valueAccessor();
-            config.change = function (value) {
-                if (value == null) {
-                    value = '';
-                }
-                config.value(value.toString());
-            };
-            config.hide = function (value) {
-                if (value == null) {
-                    value = '';
-                }
-                config.value(value.toString());
-            };
-            $(element).spectrum(config);
+
+            if (!viewModel.disabled()) {
+                config.change = function (value) {
+                    if (value == null) {
+                        value = '';
+                    }
+                    config.value(value.toString());
+                };
+
+                config.hide = function (value) {
+                    if (value == null) {
+                        value = '';
+                    }
+                    config.value(value.toString());
+                };
+                config.show = function () {
+                    if (!viewModel.focused()) {
+                        viewModel.focused(true);
+                    }
+                    return true;
+                };
+                $(element).spectrum(config);
+            } else {
+                $(element).spectrum({
+                    disabled: true
+                });
+            }
         },
 
         update: function(element, valueAccessor, allBindings, viewModel) {
             var config = valueAccessor();
-            $(element).spectrum("set", config.value());
+            if (tinycolor(config.value()).isValid() || config.value() === '') {
+                $(element).spectrum("set", config.value());
+            }
         }
     };
 
diff --git a/app/code/Magento/Ui/view/base/web/templates/form/element/colorPicker.html b/app/code/Magento/Ui/view/base/web/templates/form/element/colorPicker.html
@@ -7,9 +7,9 @@
 <div class="admin__field"
      visible="visible"
      css="$data.additionalClasses">
-    <input type="hidden" class="colorpicker-spectrum" data-bind="colorPicker: colorPickerConfig" />
+    <input type="hidden" class="colorpicker-spectrum" data-bind="colorPicker: colorPickerConfig, attr: { disabled: disabled }" />
     <input type="text" class="colorpicker-input" data-bind="
         value: value,
-        hasFocus: true,
-        attr: { placeholder: placeholder }" />
+        hasFocus: focused,
+        attr: { id: uid, placeholder: placeholder, disabled: disabled }" />
 </div>
diff --git a/app/design/adminhtml/Magento/backend/Magento_Ui/web/css/source/module/spectrum.less b/app/design/adminhtml/Magento/backend/Magento_Ui/web/css/source/module/spectrum.less
@@ -294,7 +294,7 @@ See http://bgrins.github.io/spectrum/themes/ for instructions.
 }
 .sp-input {
   font-size: 12px !important;
-  border: 1px solid #9d9d9d;
+  border: 1px solid @button__border-color;
   padding: 4px 5px;
   margin: 5px 0px 0px 0px;
   width: 100%;
@@ -341,7 +341,7 @@ See http://bgrins.github.io/spectrum/themes/ for instructions.
   border:solid 2px transparent;
 }
 .sp-palette .sp-thumb-el:hover, .sp-palette .sp-thumb-el.sp-thumb-active {
-  border-color: orange;
+  border-color: @button__hover__border-color;
 }
 .sp-thumb-el {
   position:relative;
@@ -381,7 +381,7 @@ See http://bgrins.github.io/spectrum/themes/ for instructions.
   display:inline-block;
   *zoom: 1;
   *display: inline;
-  border: solid 1px #9d9d9d;
+  border: solid 1px @button__border-color;
   background: #eaeaea;
   color: #333;
   vertical-align: middle;
@@ -407,7 +407,7 @@ See http://bgrins.github.io/spectrum/themes/ for instructions.
   position:relative;
   width: 25px;
   height: 25px;
-  border: solid 1px #9d9d9d;
+  border: solid 1px @button__border-color;
   float:left;
   z-index: 0;
 }
@@ -426,51 +426,31 @@ See http://bgrins.github.io/spectrum/themes/ for instructions.
 .sp-container {
   padding-bottom:0;
   box-shadow: 1px 1px 5px rgba(0,0,0,0.5);
+  margin-top: 4px;
 }
 
 
 /* Buttons: http://hellohappy.org/css3-buttons/ */
 .sp-container button {
-  background-color: #dcdcdc;
-  //background-image: -webkit-linear-gradient(top, #eeeeee, #cccccc);
-  //background-image: -moz-linear-gradient(top, #eeeeee, #cccccc);
-  //background-image: -ms-linear-gradient(top, #eeeeee, #cccccc);
-  //background-image: -o-linear-gradient(top, #eeeeee, #cccccc);
-  //background-image: linear-gradient(to bottom, #eeeeee, #cccccc);
-  border: 1px solid #9f9d9c;
-  //border-bottom: 1px solid #bbb;
-  //border-radius: 3px;
+  background-color: @button__background-color;
+  border: 1px solid @button__border-color;
   color: #333;
-  font-size: 14px;
-  line-height: 1;
+  font-size: @button__font-size;
+  line-height: @button__line-height;
   padding: 5px 4px;
   text-align: center;
-  //text-shadow: 0 1px 0 #eee;
   vertical-align: middle;
 }
 .sp-container button:hover {
-  background-color: #dddddd;
-  //background-image: -webkit-linear-gradient(top, #dddddd, #bbbbbb);
-  //background-image: -moz-linear-gradient(top, #dddddd, #bbbbbb);
-  //background-image: -ms-linear-gradient(top, #dddddd, #bbbbbb);
-  //background-image: -o-linear-gradient(top, #dddddd, #bbbbbb);
-  //background-image: linear-gradient(to bottom, #dddddd, #bbbbbb);
-  border: 1px solid #9f9d9c;
-  border-bottom: 1px solid #999;
+  background-color: @button__hover__background-color;
+  border: 1px solid @button__border-color;
   cursor: pointer;
-  //text-shadow: 0 1px 0 #ddd;
 }
 .sp-container button:active {
   border: 1px solid #aaa;
-  //border-bottom: 1px solid #888;
-  //-webkit-box-shadow: inset 0 0 5px 2px #aaaaaa, 0 1px 0 0 #eeeeee;
-  //-moz-box-shadow: inset 0 0 5px 2px #aaaaaa, 0 1px 0 0 #eeeeee;
-  //-ms-box-shadow: inset 0 0 5px 2px #aaaaaa, 0 1px 0 0 #eeeeee;
-  //-o-box-shadow: inset 0 0 5px 2px #aaaaaa, 0 1px 0 0 #eeeeee;
-  //box-shadow: inset 0 0 5px 2px #aaaaaa, 0 1px 0 0 #eeeeee;
 }
 .sp-cancel {
-  font-size: 11px;
+  font-size: @button__font-size;
   color: #1979c3 !important;
   margin:0;
   padding:2px;
@@ -520,5 +500,9 @@ See http://bgrins.github.io/spectrum/themes/ for instructions.
 }
 
 .sp-palette-row-selection {
-  padding-top: 25px;
+  padding-top: 10px;
+}
+
+.sp-palette-row-selection:before {
+  content: "Favorite Colors";
 }

Original file line number	Diff line number	Diff line change
`@@ -18,7 +18,7 @@ class FullMode implements ModeInterface`
`18`	`18`	`*`
`19`	`19`	`* @return array`
`20`	`20`	`*/`
`21`		`- public function getConfig()`
	`21`	`+ public function getConfig(): array`
`22`	`22`	`{`
`23`	`23`	`return [`
`24`	`24`	`'showInput' => true,`
Original file line number	Diff line number	Diff line change
`@@ -9,7 +9,7 @@`
`9`	`9`	`namespace Magento\Ui\Model\ColorPicker;`
`10`	`10`
`11`	`11`	`/**`
`12`		`- * Returns config parameters for full mode`
	`12`	`+ * Returns config parameters for noalpha mode`
`13`	`13`	`*/`
`14`	`14`	`class NoAlphaMode implements ModeInterface`
`15`	`15`	`{`
`@@ -18,7 +18,7 @@ class NoAlphaMode implements ModeInterface`
`18`	`18`	`*`
`19`	`19`	`* @return array`
`20`	`20`	`*/`
`21`		`- public function getConfig()`
	`21`	`+ public function getConfig(): array`
`22`	`22`	`{`
`23`	`23`	`return [`
`24`	`24`	`'showInput' => true,`