From 66a20271a3bd2f96e250f2b80e383b5b54298a7d Mon Sep 17 00:00:00 2001 From: Jun Tan Date: Fri, 24 Aug 2018 16:37:17 -0500 Subject: [PATCH] #200: fix broken tests, refactor semantics of input column singular + plural --- .../src/js/computed_column.js | 147 ++++++++++-------- packages/perspective-viewer/src/js/row.js | 2 +- packages/perspective-viewer/src/js/view.js | 8 +- .../test/js/computed_column_tests.js | 47 +++--- .../test/results/results.json | 15 +- packages/perspective/src/js/perspective.js | 17 +- packages/perspective/test/js/constructors.js | 2 +- 7 files changed, 128 insertions(+), 110 deletions(-) diff --git a/packages/perspective-viewer/src/js/computed_column.js b/packages/perspective-viewer/src/js/computed_column.js index 6a77968a95..7056cf0d95 100644 --- a/packages/perspective-viewer/src/js/computed_column.js +++ b/packages/perspective-viewer/src/js/computed_column.js @@ -72,7 +72,12 @@ class ComputedColumn extends HTMLElement { 'lowercase': new Computation('lowercase', 'string', 'string', x => x.toLowerCase()), 'length': new Computation('length', 'string', 'integer', x => x.length), 'add': new Computation('add', 'integer', 'integer', (a, b) => a + b, 2), + 'subtract': new Computation('subtract', 'integer', 'integer', (a, b) => a - b, 2), 'multiply': new Computation('multiply', 'integer', 'integer', (a, b) => a * b, 2), + 'divide': new Computation('divide', 'integer', 'integer', (a, b) => a / b, 2), + 'percent_a_of_b': new Computation('percent_a_of_b', 'integer', 'integer', (a, b) => ((a / b) * 100), 2), + 'concat_space': new Computation('concat_space', 'string', 'string', (a, b) => a + ' ' + b, 2), + 'concat_comma': new Computation('concat_comma', 'string', 'string', (a, b) => a + ', ' + b, 2), }; this.type_markers = { @@ -96,17 +101,19 @@ class ComputedColumn extends HTMLElement { this._computation_selector.innerHTML = ""; let iterate = true; for (let comp of Object.keys(this.computations)) { - this._computation_selector.innerHTML += ``; + this._computation_selector.innerHTML += + ``; iterate = false; } } + // Generate input column holders, reset input column state _register_inputs() { this._input_columns.innerHTML = ''; const computation = this.state.computation; const input_type = computation.input_type; - this.state.input_column = []; + this.state.input_columns = []; for (let i = 0; i < computation.num_params; i++) { this._input_columns.innerHTML += @@ -121,62 +128,40 @@ class ComputedColumn extends HTMLElement { } for (let column of this._input_columns.children) { - column.addEventListener('drop', this.column_drop.bind(this)); - column.addEventListener('dragend', this.column_undrag.bind(this)); - column.addEventListener('dragover', this.column_dragover.bind(this)); - column.addEventListener('dragleave', this.column_dragleave.bind(this)); + column.addEventListener('drop', this.drop_column.bind(this)); + column.addEventListener('dragend', this.remove_column.bind(this)); + column.addEventListener('dragover', this.hover_column.bind(this)); + column.addEventListener('dragleave', this.pass_column.bind(this)); } this._clear_column_name(); } // Drag & Drop - - // Called on end of drag operation by releasing the mouse - column_undrag(event) { - event.currentTarget.remove('dropping'); - const refresh = this._input_columns.children.length < this.state.computation.num_params; - if (refresh) - this._register_inputs(); - } - - // Called when the column leaves the target - column_dragleave(event) { - const src = event.currentTarget; - if (src !== null && src.nodeName !== 'SPAN') { - const drop_target_hover = src.querySelector('.psp-cc-computation__drop-target-hover'); - src.classList.remove('dropping'); - if(drop_target_hover) - drop_target_hover.removeAttribute('drop-target'); - } - } - - // Called when column is held over the target - column_dragover(event) { + hover_column(event) { event.preventDefault(); event.dataTransfer.dropEffect = 'move'; - const input_column = event.currentTarget; - const drop_target_hover = input_column.querySelector('.psp-cc-computation__drop-target-hover'); + const drop_target = event.currentTarget; + const drop_target_hover = drop_target.querySelector('.psp-cc-computation__drop-target-hover'); this._clear_error_messages(); - if (input_column.className !== 'dropping') { + if (drop_target.className !== 'dropping') { //event.currentTarget.classList.remove('dropped'); - input_column.classList.add('dropping'); + drop_target.classList.add('dropping'); } if (drop_target_hover && !drop_target_hover.hasAttribute('drop-target')) { - drop_target_hover.setAttribute('drop-target', true); + drop_target_hover.setAttribute('drop-target', 'true'); } - if(input_column.children.length === 2) { + if(drop_target.children.length === 2) { // drop_target_hover is the blue box - input_column.parentNode.insertBefore(drop_target_hover, input_column.nextSibling); + drop_target.parentNode.insertBefore(drop_target_hover, drop_target.nextSibling); } } - // Called when the column is dropped on the target - column_drop(event) { + drop_column(event) { event.preventDefault(); event.currentTarget.classList.remove('dropping'); @@ -190,11 +175,42 @@ class ComputedColumn extends HTMLElement { this._set_input_column(event, column_name, column_type); } - /* _apply_state() { - this._update_computation(null, this.state.computation.name); - this._set_input_column(null, this.state.input_column.name, this.state.input_column.type); - this._column_name_input.innerText = this.state.column_name; - } */ + // Called when a column is dragged out of the computed column UI + remove_column(event) { + event.currentTarget.remove('dropping'); + const refresh = this._input_columns.children.length < this.state.computation.num_params; + if (refresh) + this._register_inputs(); + } + + // Called when the column passes over and then leaves the drop target + pass_column(event) { + const src = event.currentTarget; + if (src !== null && src.nodeName !== 'SPAN') { + const drop_target_hover = src.querySelector('.psp-cc-computation__drop-target-hover'); + src.classList.remove('dropping'); + if(drop_target_hover) + drop_target_hover.removeAttribute('drop-target'); + } + } + + // When state changes are made manually, apply them to the UI + _apply_state(columns, computation, name) { + this._update_computation(null, computation.name); + this.state['input_columns'] = columns; + const inputs = this._input_columns.children; + + for (let i = 0; i < this.state['input_columns'].length; i++) { + this._set_input_column( + { currentTarget: inputs[i] }, + this.state['input_columns'][i].name, + this.state['input_columns'][i].type); + } + + this._column_name_input.innerText = name; + this._set_column_name(); + this.state['name_edited'] = true; + } // State validation _is_valid_state() { @@ -203,11 +219,15 @@ class ComputedColumn extends HTMLElement { } // error handling - _set_error_message(message, target) { - target.innerText = message; + _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 = ''; } @@ -219,13 +239,13 @@ class ComputedColumn extends HTMLElement { this._clear_error_messages(); } - _auto_name() { + _auto_column_name() { if (this.state.name_edited) { return; } - if (this.state.input_column.length > 0) { + if (this.state.input_columns.length > 0) { let names = []; - for (let column of this.state.input_column) + for (let column of this.state.input_columns) names.push(column.name); this._column_name_input.innerText = `${this.state.computation.name}(${names.join(', ')})`; } else { @@ -235,7 +255,6 @@ class ComputedColumn extends HTMLElement { } _clear_column_name() { - // TODO: clean up data flow const input = this._column_name_input; input.innerText = ''; this.state['name_edited'] = false; @@ -245,16 +264,15 @@ class ComputedColumn extends HTMLElement { _set_input_column(event, name, type) { const computation = this.state.computation; const computation_type = computation.input_type; - const inputs = this.state.input_column; + const inputs = this.state.input_columns; const target = event.currentTarget; const index = Number.parseInt(target.getAttribute('data-index')); if (type !== computation_type) { this._register_inputs(); - this._set_error_message( - `Input column type (${type}) must match computation input type (${computation_type}).`, - this._input_column_error_message); + 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; } @@ -267,24 +285,24 @@ class ComputedColumn extends HTMLElement { target.innerHTML = ''; - const input_column = { + const column = { name: name, type: type, }; if (inputs[index]) { - inputs[index] = input_column; + inputs[index] = column; } else { - inputs.push(input_column); + inputs.push(column); } - this.state['input_column'] = inputs; - this._auto_name(); + this.state['input_columns'] = inputs; + this._auto_column_name(); this.dispatchEvent(new CustomEvent('perspective-computed-column-update', { detail: { target, - input_column + column } })); } @@ -310,10 +328,9 @@ class ComputedColumn extends HTMLElement { this._computation_type.innerHTML = `${this.type_markers[return_type]}`; this.state['computation'] = computation; - this._clear_column_name(); + this._clear_column_name(); this._register_inputs(); - this._clear_error_messages(); } @@ -321,12 +338,9 @@ class ComputedColumn extends HTMLElement { _edit_computed_column(data) { this.state['computation'] = data.computation.name) this.state('column_name', data.column_name); - this.state['input_column'] = { - name: data.input_column, - type: data.input_type - }; + this.state['input_columns'] = data.input_columns, this.state['edit'] = true; - this.state['name_edited'] = data.column_name !== `${data.computation.name}(${data.input_column})`; + //this.state['name_edited'] = data.column_name !== `${data.computation.name}(${data.input_column})`; this._apply_state(); //this.classList.add('edit'); } @@ -349,7 +363,8 @@ class ComputedColumn extends HTMLElement { // save _save_computed_column() { if(!this._is_valid_state()) { - this._set_error_message('Missing parameters for computed column.', this._save_error_message); + this.state.errors.save = 'Missing parameters for computed column.'; + this._set_error_message('save', this._save_error_message); return; } diff --git a/packages/perspective-viewer/src/js/row.js b/packages/perspective-viewer/src/js/row.js index 6ab29ee747..d7410442b5 100644 --- a/packages/perspective-viewer/src/js/row.js +++ b/packages/perspective-viewer/src/js/row.js @@ -217,7 +217,7 @@ class Row extends HTMLElement { const data = JSON.parse(this.getAttribute('computed_column')); return { column_name: data.column_name, - input_column: data.input_column, + input_columns: data.input_columns, input_type: data.input_type, computation: data.computation, type: data.type, diff --git a/packages/perspective-viewer/src/js/view.js b/packages/perspective-viewer/src/js/view.js index bcf0f6f3c1..c3242550cb 100755 --- a/packages/perspective-viewer/src/js/view.js +++ b/packages/perspective-viewer/src/js/view.js @@ -319,7 +319,7 @@ function set_aggregate_attribute(aggs) { function _format_computed_data(cc) { return { column_name: cc[0], - input_column: cc[1].input_column, + input_columns: cc[1].input_columns, input_type: cc[1].input_type, computation: cc[1].computation, type: cc[1].type @@ -857,8 +857,8 @@ class ViewPrivate extends HTMLElement { _set_computed_column_input(event) { event.detail.target.appendChild( new_row.call(this, - event.detail.input_column.name, - event.detail.input_column.type)); + event.detail.column.name, + event.detail.column.type)); this._update_column_view(); } @@ -876,7 +876,7 @@ class ViewPrivate extends HTMLElement { computation: data.computation, column: computed_column_name, func: data.computation.func, - inputs: data.input_column.map(col => col.name), + inputs: data.input_columns.map(col => col.name), input_type: data.computation.input_type, type: data.computation.return_type, }]; diff --git a/packages/perspective-viewer/test/js/computed_column_tests.js b/packages/perspective-viewer/test/js/computed_column_tests.js index 672aaa2fce..1b33584f73 100644 --- a/packages/perspective-viewer/test/js/computed_column_tests.js +++ b/packages/perspective-viewer/test/js/computed_column_tests.js @@ -13,15 +13,9 @@ const add_computed_column = async (page) => { await page.evaluate(element => element.setAttribute('columns', '["Row ID","Quantity"]'), viewer); await page.click('#add-computed-column'); await page.$eval('perspective-computed-column', element => { - element._set_state('input_column', { - name: 'Order Date', - type: 'date' - }); - element._set_state('column_name', 'new_cc'); - element._set_state('name_edited', true); - element._apply_state(); + const columns = [{name: 'Order Date', type: 'date'}]; + element._apply_state(columns, element.computations['day_of_week'], 'new_cc'); }); - await page.select('#psp-cc-computation__select', 'day_of_week'); await page.click('#psp-cc-button-save'); await page.waitForSelector('perspective-viewer:not([updating])'); await page.evaluate(element => element.setAttribute('aggregates', '{"new_cc":"dominant"}'), viewer); @@ -58,33 +52,38 @@ exports.default = function() { await page.click('#add-computed-column'); await page.$eval('perspective-computed-column', element => { // call internal APIs to bypass drag/drop action - element._set_state('input_column', { - name: 'Order Date', - type: 'date', - }); - element._set_state('column_name', 'new_cc'); - element._set_state('name_edited', true); - element._apply_state(); + const columns = [{name: 'State', type: 'string'}]; + element._apply_state(columns, element.computations['lowercase'], 'new_cc'); + }); + }); + + test.capture("setting multiple column parameters should set input.", async page => { + await page.click('#config_button'); + const viewer = await page.$('perspective-viewer'); + await page.evaluate(element => element.setAttribute('columns', '["Row ID","Quantity"]'), viewer); + await page.$('perspective-viewer'); + await page.click('#add-computed-column'); + await page.$eval('perspective-computed-column', element => { + const columns = [ + {name: 'Quantity', type: 'integer'}, + {name: 'Row ID', type: 'integer'} + ]; + element._apply_state(columns, element.computations['add'], 'new_cc'); }); }); // computation - test.capture("computations of the same type should not clear input column.", async page => { + test.capture("computations should clear input column.", async page => { await page.click('#config_button'); const viewer = await page.$('perspective-viewer'); await page.evaluate(element => element.setAttribute('columns', '["Row ID","Quantity"]'), viewer); await page.$('perspective-viewer'); await page.click('#add-computed-column'); await page.$eval('perspective-computed-column', element => { - element._set_state('input_column', { - name: 'Order Date', - type: 'date', - }); - element._set_state('column_name', 'new_cc'); - element._set_state('name_edited', true); - element._apply_state(); + const columns = [{name: 'State', type: 'string'}]; + element._apply_state(columns, element.computations['lowercase'], 'new_cc'); }); - await page.select('#psp-cc-computation__select', 'day_of_week'); + await page.select('#psp-cc-computation__select', 'subtract'); }); // save diff --git a/packages/perspective-viewer/test/results/results.json b/packages/perspective-viewer/test/results/results.json index 92eb4d3089..90fbcbf4f9 100644 --- a/packages/perspective-viewer/test/results/results.json +++ b/packages/perspective-viewer/test/results/results.json @@ -10,19 +10,20 @@ "superstore.html/sorts by a hidden column.": "57bd2b3739ab342c71d6878722db2d95", "superstore.html/filters by a numeric column.": "dc1596fb2db82243f2200a0b9d8d35b8", "superstore.html/shows horizontal columns on small viewports.": "a182af50a17d94732c8fa6540fc56233", - "superstore.html/setting a valid column should set it as input.": "7ab0c6a33a4bfa826edf8d2a2f2fe5a2", - "superstore.html/computations of the same type should not clear input column.": "f608917490556d33c44d291960163aa4", - "superstore.html/saving a computed column should add it to inactive columns.": "60e618267929f77003dad9fd9f337577", + "superstore.html/setting a valid column should set it as input.": "7e9051e056a2723f886f7ecd340fd068", + "superstore.html/setting multiple column parameters should set input.": "162a2bd64e307b628c6e7423f55b85dc", + "superstore.html/computations should clear input column.": "894575c747a636c1de8c7616acb63357", + "superstore.html/saving a computed column should add it to inactive columns.": "a831f7659b423d5051af088cf2208e18", "superstore.html/aggregates by computed column.": "f9ba5b75dd267d3dddca65976fbd63f2", "superstore.html/pivots by computed column.": "d4467ad3612e90136fa391196e61189b", - "superstore.html/sorts by computed column.": "7a4c989e68159de9f2ed70a1e6380ef9", - "superstore.html/filters by computed column.": "12d31929677602612fdb26ee04958eef", "superstore.html/computed column aggregates should persist.": "0dbc9d15dc1a9d62d62e94b534447787", + "superstore.html/sorts by computed column.": "4daa6067842eecdddb9f3c06fbbb4326", + "superstore.html/filters by computed column.": "129c3afb5027f2e3454addcc212cd13d", "blank.html/Handles reloading with a schema.": "9cda0b27c92efb59599e11dffbad169e", "superstore.html/pivots by a column.": "71c2f17f6ade6513574aa0143a84c634", - "superstore.html/click on add computed column button opens the UI.": "4dd9778ed1f321d928570d7b3e60a473", + "superstore.html/click on add computed column button opens the UI.": "84bec558c77c5d54a608a135f3e7f9c2", "superstore.html/click on close button closes the UI.": "c10ffcce1cc9d93fdbcaeccfc12c7ca7", - "superstore.html/saving without parameters should show an error message.": "1d22204b783657c99cf03f2e862224ac", + "superstore.html/saving without parameters should show an error message.": "657c4319e6177c16880c38e8cccfaf30", "superstore.html/doesn't leak tables.": "f60686399c38b6154be75b3ebed74c83", "superstore.html/doesn't leak views when setting row pivots.": "9b51f44a534c4d49e4ad3876afacda0c", "superstore.html/doesn't leak views when setting filters.": "e67e550265c4256b52788e2a27d572b0" diff --git a/packages/perspective/src/js/perspective.js b/packages/perspective/src/js/perspective.js index 68ab91265d..6f916bc048 100644 --- a/packages/perspective/src/js/perspective.js +++ b/packages/perspective/src/js/perspective.js @@ -901,8 +901,6 @@ table.prototype._calculate_computed = function(tbl, computed_defs) { let inputs = coldef['inputs']; let type = coldef['type'] || 'string'; - console.log(coldef, name, func, inputs, type); - let dtype; switch (type) { case 'integer': @@ -1016,7 +1014,7 @@ table.prototype._computed_schema = function() { const column = {}; column.type = column_type; - column.input_column = computed[i].inputs; // edit to support multiple input columns + column.input_columns = computed[i].inputs; column.input_type = computed[i].input_type; column.computation = computed[i].computation; @@ -1454,7 +1452,7 @@ table.prototype._column_metadata = function () { if (computed_col !== undefined) { meta.computed = { - input_column: computed_col.input_column, + input_columns: computed_col.input_columns, input_type: computed_col.input_type, computation: computed_col.computation } @@ -1473,9 +1471,14 @@ table.prototype._column_metadata = function () { } /** - * Column metadata for this table. If the column is computed, the `computed` property - * is an Object containing `input_column`, `input_type`, and `computation`. Otherwise, - * `computed` is `undefined`. + * Column metadata for this table. + * + * If the column is computed, the `computed` property is an Object containing: + * - Array `input_columns` + * - String `input_type` + * - Object `computation`. + * + * Otherwise, `computed` is `undefined`. * * @async * diff --git a/packages/perspective/test/js/constructors.js b/packages/perspective/test/js/constructors.js index f692112017..46f1afd9fc 100644 --- a/packages/perspective/test/js/constructors.js +++ b/packages/perspective/test/js/constructors.js @@ -366,7 +366,7 @@ module.exports = (perspective) => { const result = await table2.computed_schema(); const expected = { "plus2": { - input_column: "x", + input_columns: ["x"], input_type: "integer", computation: computation, type: "integer"