Fix computed column UX issues

packages/perspective-viewer/package.json

@@ -16,6 +16,7 @@
         "build:webpack:cjs": "webpack --color --config src/config/view.cjs.config.js",
         "build:webpack:viewer": "webpack --color --config src/config/view.config.js",
         "build": "npm-run-all build:webpack:* build:themes:* copy:*",
+        "watch": "webpack --color --watch --config src/config/view.config.js",
         "copy:worker": "cp -r node_modules/@jpmorganchase/perspective/build/*.worker.*.js build",
         "copy:sourcemaps": "cp -r node_modules/@jpmorganchase/perspective/build/*.worker.*.js.map build",
         "copy:wasm": "cp -r node_modules/@jpmorganchase/perspective/build/psp.async.wasm build",

packages/perspective-viewer/src/html/computed_column.html

@@ -16,7 +16,10 @@
         <div class="psp-cc__container" style="margin-top:-7px;">
             <div class="psp-cc__content">
                 <div id="psp-cc-computation__type"></div>
-                <span contentEditable=true type="text" required maxlength="25" size="10" autocomplete="off" id="psp-cc-name" ondragover="disallowDrop(event)" />
+                <span contentEditable=true type="text" required maxlength="25" size="10" autocomplete="off" id="psp-cc-name" ondragover="disallowDrop(event)"></span>
+            </div>
+            <div class="psp-cc__content psp-cc__content--nomargin">
+                <span class="psp-cc__label psp-cc__error" id="psp-cc__error--name"></span>
             </div>
         </div>
         <div class="psp-cc__container">
@@ -26,17 +29,13 @@
             </div>
         </div>
         <div class="psp-cc__container" style="margin-top:-12px;">
-            <span id="psp-cc__error--input" class="psp-cc__label psp-cc__error"></span>
             <div id="psp-cc-computation-inputs">
                 <!--<div class="psp-cc-computation__input-column" drop-target ondragenter="dragEnter(event)"></div>-->
             </div>
         </div>
-        <div class="psp-cc__container">
-            <span id="psp-cc__error--save" class="psp-cc__label psp-cc__error"></span>
-        </div>
     </div>
     <div id="psp-cc__actions">
         <button id="psp-cc-button-delete" class="psp-cc__button">Delete</button>
-        <button id="psp-cc-button-save" class="psp-cc__button">Save</button>
+        <button id="psp-cc-button-save" class="psp-cc__button" disabled>Save</button>
     </div>
 </template>

packages/perspective-viewer/src/js/computed_column.js

@@ -19,12 +19,6 @@ import style from "../less/computed_column.less";
 
 polyfill({});
 
-/******************************************************************************
- *
- * Drag & Drop Utils
- *
- */
-
 // Computations
 const hour_of_day = function(val) {
     return new Date(val).getHours();
@@ -101,13 +95,15 @@ class ComputedColumn extends HTMLElement {
         super();
 
         this.state = new State();
+        this.column_names = [];
 
         this.type_markers = {
             float: "123",
             integer: "123",
             string: "abc",
             boolean: "t/f",
-            datetime: "mdy"
+            datetime: "mdy",
+            date: "mdy"
         };
     }
 
@@ -134,20 +130,19 @@ class ComputedColumn extends HTMLElement {
 
     // Generate input column holders, reset input column state
     _register_inputs() {
+        this._clear_error_messages();
+        this._disable_save_button();
         this._input_columns.innerHTML = "";
         const computation = this.state.computation;
         const input_type = computation.input_type;
 
         this.state.input_columns = [];
         this.state.swap_target = false;
 
-        // todo: replace html-loader with handlebars-loader
-
         for (let i = 0; i < computation.num_params; i++) {
             this._input_columns.innerHTML += `<div class="psp-cc-computation__input-column" 
                       data-index="${i}" 
-                      drop-target 
-                      ondragenter="dragEnter(event)">
+                      drop-target>
                       <span class="psp-label__requiredType ${input_type}"></span>
                       <span class="psp-label__placeholder">Param ${i + 1}</span>
                       <div class="psp-cc-computation__drop-target-hover"></div>
@@ -190,9 +185,6 @@ class ComputedColumn extends HTMLElement {
 
         const drop_target = event.currentTarget;
         const drop_target_hover = drop_target.querySelector(".psp-cc-computation__drop-target-hover");
-
-        this._clear_error_messages();
-
         if (drop_target.className !== "dropping") {
             //event.currentTarget.classList.remove('dropped');
             drop_target.classList.add("dropping");
@@ -249,9 +241,16 @@ class ComputedColumn extends HTMLElement {
     // Called when the column passes over and then leaves the drop target
     _pass_column(event) {
         const src = event.currentTarget;
+        // are we inside the column? if we are, prevent further calls which cause flickering
+        const bounds = src.getBoundingClientRect();
+        const inside_x = event.pageX >= bounds.left && event.pageX <= bounds.right - 2;
+        const inside_y = event.pageY >= bounds.top && event.pageY <= bounds.bottom - 2;
+        if (inside_x && inside_y) {
+            return;
+        }
         if (src !== null && src.nodeName !== "SPAN") {
-            const drop_target_hover = src.querySelector(".psp-cc-computation__drop-target-hover");
             src.classList.remove("dropping");
+            const drop_target_hover = src.querySelector(".psp-cc-computation__drop-target-hover");
             if (drop_target_hover) drop_target_hover.removeAttribute("drop-target");
         }
     }
@@ -271,27 +270,26 @@ class ComputedColumn extends HTMLElement {
         this._column_name_input.innerText = name || "";
         this._set_column_name();
         this.state["name_edited"] = name !== undefined;
-    }
-
-    // error handling
-    _set_error_message(type, target) {
-        target.innerText = this.state.errors[type];
-    }
 
-    _clear_error_messages() {
-        this.state["errors"] = {
-            input_column: undefined,
-            save: undefined
-        };
-        this._input_column_error_message.innerText = "";
-        this._save_error_message.innerText = "";
+        if (this.state.is_valid()) {
+            this._enable_save_button();
+        }
     }
 
     // column_name
     _set_column_name() {
         const input = this._column_name_input;
-        this.state["column_name"] = input.innerText;
-        this._clear_error_messages();
+        let name = input.innerText;
+        if (name.length == 0) {
+            this.state["column_name"] = undefined;
+            this._disable_save_button();
+            return;
+        }
+        this.state["column_name"] = name;
+
+        if (this.state.is_valid()) {
+            this._enable_save_button();
+        }
     }
 
     _auto_column_name() {
@@ -315,6 +313,7 @@ class ComputedColumn extends HTMLElement {
         this._set_column_name();
     }
 
+    // input column
     _set_input_column(event, name, type) {
         const computation = this.state.computation;
         const computation_type = computation.input_type;
@@ -335,8 +334,6 @@ class ComputedColumn extends HTMLElement {
             (computation_type === "datetime" && type !== "datetime" && type !== "date")
         ) {
             this._register_inputs();
-            this.state.errors.input_column = `Input column type (${type}) must match computation input type (${computation_type}).`;
-            this._set_error_message("input_column", this._input_column_error_message);
             target.classList.remove("dropped");
             return;
         }
@@ -368,6 +365,10 @@ class ComputedColumn extends HTMLElement {
                 }
             })
         );
+
+        if (this.state.is_valid()) {
+            this._enable_save_button();
+        }
     }
 
     // computation
@@ -386,36 +387,73 @@ class ComputedColumn extends HTMLElement {
             throw "Undefined computation could not be set.";
         }
 
+        const num_params = computation.num_params;
+        const input_type = computation.input_type;
         const return_type = computation.return_type;
+        let reset_inputs = true;
+
+        if (this.state["computation"]) {
+            // do we need to reset the input? if types/num_params differ then yes
+            reset_inputs = input_type !== this.state["computation"].input_type || num_params !== this.state["computation"].num_params;
+        }
 
         this._computation_type.innerHTML = `<span class="${return_type}">${this.type_markers[return_type]}</span>`;
 
         this.state["computation"] = computation;
 
-        this._clear_column_name();
-        this._register_inputs();
-        this._clear_error_messages();
+        if (reset_inputs || event === null) {
+            this._register_inputs();
+            this._clear_column_name();
+        } else {
+            this._auto_column_name();
+        }
     }
 
-    // save
-    _save_computed_column() {
-        if (!this.state.is_valid()) {
-            this.state.errors.save = "Missing parameters for computed column.";
-            this._set_error_message("save", this._save_error_message);
-            return;
+    // error message handlers
+    _set_error_message(message, target) {
+        if (target) {
+            target.innerText = message;
+            target.style.display = "block";
         }
+    }
+
+    _clear_error_messages() {
+        this._column_name_error.innerText = "";
+        this._column_name_error.style.display = "none";
+    }
+
+    // save button handlers
+    _disable_save_button() {
+        this._save_button.setAttribute("disabled", "true");
+    }
 
-        const computed_column = this.state;
+    _enable_save_button() {
+        this._save_button.removeAttribute("disabled");
+    }
+
+    // save
+    _save_computed_column() {
+        if (this.state.is_valid()) {
+            const computed_column = this.state;
 
-        const event = new CustomEvent("perspective-computed-column-save", {
-            detail: {
-                name: computed_column.column_name,
-                inputs: computed_column.input_columns.map(x => x.name),
-                func: computed_column.computation.name
+            if (this.column_names.includes(this.state.column_name)) {
+                this._set_error_message("Column names must be unique.", this._column_name_error);
+                return;
             }
-        });
 
-        this.dispatchEvent(event);
+            this._clear_error_messages();
+
+            const event = new CustomEvent("perspective-computed-column-save", {
+                detail: {
+                    name: computed_column.column_name,
+                    inputs: computed_column.input_columns.map(x => x.name),
+                    func: computed_column.computation.name
+                }
+            });
+            this.dispatchEvent(event);
+
+            this.column_names.push(computed_column.column_name);
+        }
     }
 
     // close
@@ -437,13 +475,12 @@ class ComputedColumn extends HTMLElement {
         this._side_panel_actions = this.parentElement.querySelector("#side_panel__actions");
         this._close_button = this.shadowRoot.querySelector("#psp-cc__close");
         this._column_name_input = this.shadowRoot.querySelector("#psp-cc-name");
+        this._column_name_error = this.shadowRoot.querySelector("#psp-cc__error--name");
         this._computation_selector = this.shadowRoot.querySelector("#psp-cc-computation__select");
         this._computation_type = this.shadowRoot.querySelector("#psp-cc-computation__type");
         this._input_columns = this.shadowRoot.querySelector("#psp-cc-computation-inputs");
         //this._delete_button = this.shadowRoot.querySelector('#psp-cc-button-delete');
         this._save_button = this.shadowRoot.querySelector("#psp-cc-button-save");
-        this._input_column_error_message = this.shadowRoot.querySelector("#psp-cc__error--input");
-        this._save_error_message = this.shadowRoot.querySelector("#psp-cc__error--save");
     }
 
     _register_callbacks() {

packages/perspective-viewer/src/js/computed_column/State.js

@@ -11,10 +11,6 @@ import {values} from "underscore";
 
 export default class State {
     constructor() {
-        this.errors = {
-            input_column: undefined,
-            save: undefined
-        };
         this.edit = false;
         this.column_name = undefined;
         this.computation = undefined;

packages/perspective-viewer/src/js/row.js

@@ -19,33 +19,6 @@ import template from "../html/row.html";
 
 import style from "../less/row.less";
 
-/******************************************************************************
- *
- * Drag & Drop Utils
- *
- */
-
-global.dragEnter = function dragEnter(ev) {
-    ev.stopPropagation();
-    ev.preventDefault();
-    ev.currentTarget.classList.add("dropping");
-};
-
-global.allowDrop = function allowDrop(ev) {
-    ev.stopPropagation();
-    ev.preventDefault();
-    ev.currentTarget.classList.add("dropping");
-    ev.dataTransfer.dropEffect = "move";
-};
-
-global.disallowDrop = function disallowDrop(ev) {
-    if (ev.currentTarget == ev.target) {
-        ev.stopPropagation();
-        ev.preventDefault();
-        ev.currentTarget.classList.remove("dropping");
-    }
-};
-
 function get_text_width(text, max = 0) {
     let span = document.createElement("span");
     // FIXME get these values form the stylesheet

packages/perspective-viewer/src/js/utils.js

@@ -7,6 +7,33 @@
  *
  */
 
+/******************************************************************************
+ *
+ * Drag & Drop Utils
+ *
+ */
+
+global.dragEnter = function dragEnter(ev) {
+    ev.stopPropagation();
+    ev.preventDefault();
+    ev.currentTarget.classList.add("dropping");
+};
+
+global.allowDrop = function allowDrop(ev) {
+    ev.stopPropagation();
+    ev.preventDefault();
+    ev.currentTarget.classList.add("dropping");
+    ev.dataTransfer.dropEffect = "move";
+};
+
+global.disallowDrop = function disallowDrop(ev) {
+    if (ev.currentTarget == ev.target) {
+        ev.stopPropagation();
+        ev.preventDefault();
+        ev.currentTarget.classList.remove("dropping");
+    }
+};
+
 /**
  * Instantiate a Template DOM object from an HTML text string.
  *