From 879c97f64c8f2765fa1ca21ece3832c7d8426e57 Mon Sep 17 00:00:00 2001 From: Josh Johnson Date: Fri, 22 Feb 2019 22:04:55 +0000 Subject: [PATCH] Resolve undefined error (#528) * Remove run-p from test command * Remove dropdown interaction tests * Tidy utils * Use merge lib * Remove string casting * Sanitise in constants * Housekeeping * Add non-string value tests --- .eslintrc | 1 + cypress/integration/select-multiple.spec.js | 74 ++++++------- cypress/integration/select-one.spec.js | 76 ++++++------- package.json | 2 +- public/test/select-multiple.html | 46 ++++++-- public/test/select-one.html | 42 +++++-- src/scripts/choices.js | 3 +- src/scripts/components/input.js | 10 +- src/scripts/components/wrapped-input.js | 9 +- src/scripts/constants.js | 4 +- src/scripts/lib/utils.js | 117 ++++++-------------- src/scripts/lib/utils.test.js | 74 +------------ 12 files changed, 184 insertions(+), 274 deletions(-) diff --git a/.eslintrc b/.eslintrc index 8c2836b..185a527 100644 --- a/.eslintrc +++ b/.eslintrc @@ -42,6 +42,7 @@ "no-plusplus": "off", "no-unused-expressions": "off", "no-underscore-dangle": "off", + "consistent-return": "off", "prettier/prettier": [ "error", { diff --git a/cypress/integration/select-multiple.spec.js b/cypress/integration/select-multiple.spec.js index ade66bb..1e46fa3 100644 --- a/cypress/integration/select-multiple.spec.js +++ b/cypress/integration/select-multiple.spec.js @@ -68,7 +68,7 @@ describe('Choices - select multiple', () => { .click(); }); - it('allows me select choices from a dropdown', () => { + it('allows selecting choices from dropdown', () => { cy.get('[data-test-hook=basic]') .find('.choices__list--multiple .choices__item') .last() @@ -211,44 +211,6 @@ describe('Choices - select multiple', () => { }); }); - /* - There is currently a bug with opening/closing/toggling dropdowns - - @todo Investigate why - */ - describe('interacting with dropdown', () => { - describe('opening dropdown', () => { - it('opens dropdown', () => { - cy.get('[data-test-hook=basic]') - .find('button.open-dropdown') - .focus() - .click(); - - cy.get('[data-test-hook=basic]') - .find('.choices__list--dropdown') - .should('be.visible'); - }); - }); - - describe('closing dropdown', () => { - it('closes dropdown', () => { - cy.get('[data-test-hook=basic]') - .find('button.open-dropdown') - .focus() - .click(); - - cy.get('[data-test-hook=basic]') - .find('button.close-dropdown') - .focus() - .click(); - - cy.get('[data-test-hook=basic]') - .find('.choices__list--dropdown') - .should('not.be.visible'); - }); - }); - }); - describe('disabling', () => { describe('on disable', () => { it('disables the search input', () => { @@ -754,6 +716,40 @@ describe('Choices - select multiple', () => { }); }); + describe('non-string values', () => { + beforeEach(() => { + cy.get('[data-test-hook=non-string-values]') + .find('.choices') + .click(); + }); + + it('displays expected amount of choices in dropdown', () => { + cy.get('[data-test-hook=non-string-values]') + .find('.choices__list--dropdown .choices__list') + .children() + .should('have.length', 4); + }); + + it('allows selecting choices from dropdown', () => { + let $selectedChoice; + cy.get('[data-test-hook=non-string-values]') + .find('.choices__list--dropdown .choices__list') + .children() + .first() + .then($choice => { + $selectedChoice = $choice; + }) + .click(); + + cy.get('[data-test-hook=non-string-values]') + .find('.choices__list--single .choices__item') + .last() + .should($item => { + expect($item.text().trim()).to.equal($selectedChoice.text().trim()); + }); + }); + }); + describe('within form', () => { describe('selecting choice', () => { describe('on enter key', () => { diff --git a/cypress/integration/select-one.spec.js b/cypress/integration/select-one.spec.js index 1ac30dc..f68d1b0 100644 --- a/cypress/integration/select-one.spec.js +++ b/cypress/integration/select-one.spec.js @@ -45,7 +45,7 @@ describe('Choices - select one', () => { describe('selecting choices', () => { const selectedChoiceText = 'Choice 1'; - it('allows me select choices from a dropdown', () => { + it('allows selecting choices from dropdown', () => { cy.get('[data-test-hook=basic]') .find('.choices__list--dropdown .choices__list') .children() @@ -129,44 +129,6 @@ describe('Choices - select one', () => { }); }); - /* - There is currently a bug with opening/closing/toggling dropdowns - - @todo Investigate why - */ - describe('interacting with dropdown', () => { - describe('opening dropdown', () => { - it('opens dropdown', () => { - cy.get('[data-test-hook=basic]') - .find('button.open-dropdown') - .focus() - .click(); - - cy.get('[data-test-hook=basic]') - .find('.choices__list--dropdown') - .should('be.visible'); - }); - }); - - describe('closing dropdown', () => { - it('closes dropdown', () => { - cy.get('[data-test-hook=basic]') - .find('button.open-dropdown') - .focus() - .click(); - - cy.get('[data-test-hook=basic]') - .find('button.close-dropdown') - .focus() - .click(); - - cy.get('[data-test-hook=basic]') - .find('.choices__list--dropdown') - .should('not.be.visible'); - }); - }); - }); - describe('disabling', () => { describe('on disable', () => { it('disables the search input', () => { @@ -418,7 +380,7 @@ describe('Choices - select one', () => { .should('not.exist'); }); - it('allows me select choices from a dropdown', () => { + it('allows selecting choices from dropdown', () => { cy.get('[data-test-hook=search-disabled]') .find('.choices__list--dropdown .choices__list') .children() @@ -766,6 +728,40 @@ describe('Choices - select one', () => { }); }); + describe('non-string values', () => { + beforeEach(() => { + cy.get('[data-test-hook=non-string-values]') + .find('.choices') + .click(); + }); + + it('displays expected amount of choices in dropdown', () => { + cy.get('[data-test-hook=non-string-values]') + .find('.choices__list--dropdown .choices__list') + .children() + .should('have.length', 4); + }); + + it('allows selecting choices from dropdown', () => { + let $selectedChoice; + cy.get('[data-test-hook=non-string-values]') + .find('.choices__list--dropdown .choices__list') + .children() + .first() + .then($choice => { + $selectedChoice = $choice; + }) + .click(); + + cy.get('[data-test-hook=non-string-values]') + .find('.choices__list--single .choices__item') + .last() + .should($item => { + expect($item.text().trim()).to.equal($selectedChoice.text().trim()); + }); + }); + }); + describe('within form', () => { describe('selecting choice', () => { describe('on enter key', () => { diff --git a/package.json b/package.json index 22d3fb2..b001c46 100644 --- a/package.json +++ b/package.json @@ -12,7 +12,7 @@ "bundlesize": "bundlesize", "cypress:run": "$(npm bin)/cypress run", "cypress:open": "$(npm bin)/cypress open", - "test": "run-p test:unit test:e2e", + "test": "npm run test:unit && npm run test:e2e", "test:unit": "mocha --require ./config/jsdom.js --require @babel/register $(find src -name '*.test.js') --exit", "test:unit:watch": "npm run test:unit -- --watch --inspect=5556", "test:e2e": "run-p --race start cypress:run", diff --git a/public/test/select-multiple.html b/public/test/select-multiple.html index 2b0b4a8..e860d20 100644 --- a/public/test/select-multiple.html +++ b/public/test/select-multiple.html @@ -31,8 +31,6 @@

Select multiple inputs

- -
+
+ + +
+
@@ -207,14 +210,6 @@ document.addEventListener('DOMContentLoaded', function() { const choicesBasic = new Choices('#choices-basic'); - document.querySelector('button.open-dropdown').addEventListener('click', () => { - choicesBasic.showDropdown(); - }); - - document.querySelector('button.close-dropdown').addEventListener('click', () => { - choicesBasic.hideDropdown(); - }); - document.querySelector('button.disable').addEventListener('click', () => { choicesBasic.disable(); }); @@ -303,8 +298,35 @@ customProperties: { country: 'Portugal', }, - } - ] + }, + ], + }); + + new Choices('#choices-non-string-values', { + choices: [ + { + id: 1, + label: 'Number', + value: 1, + }, + { + id: 2, + label: 'Boolean', + value: true, + }, + { + id: 3, + label: 'Object', + value: { + test: true, + }, + }, + { + id: 4, + label: 'Array', + value: ['test'], + }, + ], }); new Choices('#choices-within-form'); diff --git a/public/test/select-one.html b/public/test/select-one.html index b78177c..de37a75 100644 --- a/public/test/select-one.html +++ b/public/test/select-one.html @@ -31,8 +31,6 @@

Select one inputs

- -
+
+ + +
+
@@ -211,14 +214,6 @@ document.addEventListener('DOMContentLoaded', function() { const choicesBasic = new Choices('#choices-basic'); - document.querySelector('button.open-dropdown').addEventListener('click', () => { - choicesBasic.showDropdown(); - }); - - document.querySelector('button.close-dropdown').addEventListener('click', () => { - choicesBasic.hideDropdown(); - }); - document.querySelector('button.disable').addEventListener('click', () => { choicesBasic.disable(); }); @@ -319,6 +314,33 @@ ] }); + new Choices('#choices-non-string-values', { + choices: [ + { + id: 1, + label: 'Number', + value: 1, + }, + { + id: 2, + label: 'Boolean', + value: true, + }, + { + id: 3, + label: 'Object', + value: { + test: true, + }, + }, + { + id: 4, + label: 'Array', + value: ['test'], + }, + ], + }); + new Choices('#choices-within-form'); new Choices('#choices-set-choice-by-value').setChoiceByValue('Choice 2'); diff --git a/src/scripts/choices.js b/src/scripts/choices.js index bcb8096..24b028b 100644 --- a/src/scripts/choices.js +++ b/src/scripts/choices.js @@ -29,7 +29,6 @@ import { getType, isType, strToEl, - extend, sortByScore, generateId, findAncestorByAttrName, @@ -1787,7 +1786,7 @@ class Choices { userTemplates = callbackOnCreateTemplates.call(this, strToEl); } - this.config.templates = extend(TEMPLATES, userTemplates); + this.config.templates = merge(TEMPLATES, userTemplates); } _createElements() { diff --git a/src/scripts/components/input.js b/src/scripts/components/input.js index a26c0d9..f2c97be 100644 --- a/src/scripts/components/input.js +++ b/src/scripts/components/input.js @@ -1,15 +1,12 @@ -import { calcWidthOfInput, stripHTML } from '../lib/utils'; +import { calcWidthOfInput, sanitise } from '../lib/utils'; export default class Input { constructor({ element, type, classNames, placeholderValue }) { Object.assign(this, { element, type, classNames, placeholderValue }); - this.element = element; this.classNames = classNames; this.isFocussed = this.element === document.activeElement; this.isDisabled = false; - - // Bind event listeners this._onPaste = this._onPaste.bind(this); this._onInput = this._onInput.bind(this); this._onFocus = this._onFocus.bind(this); @@ -21,11 +18,11 @@ export default class Input { } set value(value) { - this.element.value = `${value}`; + this.element.value = value; } get value() { - return stripHTML(this.element.value); + return sanitise(this.element.value); } addEventListeners() { @@ -134,7 +131,6 @@ export default class Input { _onPaste(event) { const { target } = event; - // Disable pasting into the input if option has been set if (target === this.element && this.preventPaste) { event.preventDefault(); } diff --git a/src/scripts/components/wrapped-input.js b/src/scripts/components/wrapped-input.js index 283eb54..d9143ce 100644 --- a/src/scripts/components/wrapped-input.js +++ b/src/scripts/components/wrapped-input.js @@ -1,5 +1,4 @@ import WrappedElement from './wrapped-element'; -import { reduceToValues } from './../lib/utils'; export default class WrappedInput extends WrappedElement { constructor({ element, classNames, delimiter }) { @@ -8,11 +7,11 @@ export default class WrappedInput extends WrappedElement { } set value(items) { - const itemsFiltered = reduceToValues(items); - const itemsFilteredString = itemsFiltered.join(this.delimiter); + const itemValues = items.map(({ value }) => value); + const joinedValues = itemValues.join(this.delimiter); - this.element.setAttribute('value', itemsFilteredString); - this.element.value = itemsFilteredString; + this.element.setAttribute('value', joinedValues); + this.element.value = joinedValues; } // @todo figure out why we need this? Perhaps a babel issue diff --git a/src/scripts/constants.js b/src/scripts/constants.js index 73b9707..13a720d 100644 --- a/src/scripts/constants.js +++ b/src/scripts/constants.js @@ -1,4 +1,4 @@ -import { stripHTML, sortByAlpha } from './lib/utils'; +import { sanitise, sortByAlpha } from './lib/utils'; export const DEFAULT_CLASSNAMES = { containerOuter: 'choices', @@ -65,7 +65,7 @@ export const DEFAULT_CONFIG = { itemSelectText: 'Press to select', uniqueItemText: 'Only unique values can be added', customAddItemText: 'Only values matching specific conditions can be added', - addItemText: value => `Press Enter to add "${stripHTML(value)}"`, + addItemText: value => `Press Enter to add "${sanitise(value)}"`, maxItemText: maxItemCount => `Only ${maxItemCount} values can be added`, itemComparer: (choice, item) => choice === item, fuseOptions: { diff --git a/src/scripts/lib/utils.js b/src/scripts/lib/utils.js index 2eda11d..cc03117 100644 --- a/src/scripts/lib/utils.js +++ b/src/scripts/lib/utils.js @@ -1,10 +1,7 @@ -/* eslint-disable */ +export const getRandomNumber = (min, max) => + Math.floor(Math.random() * (max - min) + min); -export const getRandomNumber = function(min, max) { - return Math.floor(Math.random() * (max - min) + min); -}; - -export const generateChars = function(length) { +export const generateChars = length => { let chars = ''; for (let i = 0; i < length; i++) { @@ -15,7 +12,7 @@ export const generateChars = function(length) { return chars; }; -export const generateId = function(element, prefix) { +export const generateId = (element, prefix) => { let id = element.id || (element.name && `${element.name}-${generateChars(2)}`) || @@ -26,56 +23,14 @@ export const generateId = function(element, prefix) { return id; }; -export const getType = function(obj) { - return Object.prototype.toString.call(obj).slice(8, -1); -}; +export const getType = obj => Object.prototype.toString.call(obj).slice(8, -1); -export const isType = function(type, obj) { - const clas = getType(obj); - return obj !== undefined && obj !== null && clas === type; -}; +export const isType = (type, obj) => + obj !== undefined && obj !== null && getType(obj) === type; -export const isElement = (element) => { - return element instanceof Element; -}; +export const isElement = element => element instanceof Element; -export const extend = function() { - const extended = {}; - const length = arguments.length; - - /** - * Merge one object into another - * @param {Object} obj Object to merge into extended object - */ - const merge = function(obj) { - for (const prop in obj) { - if (Object.prototype.hasOwnProperty.call(obj, prop)) { - // If deep merge and property is an object, merge properties - if (isType('Object', obj[prop])) { - extended[prop] = extend(true, extended[prop], obj[prop]); - } else { - extended[prop] = obj[prop]; - } - } - } - }; - - // Loop through each passed argument - for (let i = 0; i < length; i++) { - // store argument at position i - const obj = arguments[i]; - - // If we are in fact dealing with an object, merge it. - if (isType('Object', obj)) { - merge(obj); - } - } - - return extended; -}; - -export const wrap = function(element, wrapper) { - wrapper = wrapper || document.createElement('div'); +export const wrap = (element, wrapper = document.createElement('div')) => { if (element.nextSibling) { element.parentNode.insertBefore(wrapper, element.nextSibling); } else { @@ -84,12 +39,7 @@ export const wrap = function(element, wrapper) { return wrapper.appendChild(element); }; -export const findAncestor = function(el, cls) { - while ((el = el.parentElement) && !el.classList.contains(cls)); - return el; -}; - -export const findAncestorByAttrName = function(el, attr) { +export const findAncestorByAttrName = (el, attr) => { let target = el; while (target) { @@ -104,7 +54,9 @@ export const findAncestorByAttrName = function(el, attr) { }; export const getAdjacentEl = (startEl, className, direction = 1) => { - if (!startEl || !className) return; + if (!startEl || !className) { + return; + } const parent = startEl.parentNode.parentNode; const children = Array.from(parent.querySelectorAll(className)); @@ -116,7 +68,9 @@ export const getAdjacentEl = (startEl, className, direction = 1) => { }; export const isScrolledIntoView = (el, parent, direction = 1) => { - if (!el) return; + if (!el) { + return; + } let isVisible; @@ -132,26 +86,30 @@ export const isScrolledIntoView = (el, parent, direction = 1) => { return isVisible; }; -export const stripHTML = html => - html +export const sanitise = value => { + if (!isType('String', value)) { + return value; + } + + return value .replace(/&/g, '&') .replace(/>/g, '&rt;') .replace(/ { const tmpEl = document.createElement('div'); - return function(str) { + return str => { const cleanedInput = str.trim(); - let r; tmpEl.innerHTML = cleanedInput; - r = tmpEl.children[0]; + const firldChild = tmpEl.children[0]; while (tmpEl.firstChild) { tmpEl.removeChild(tmpEl.firstChild); } - return r; + return firldChild; }; })(); @@ -164,7 +122,7 @@ export const calcWidthOfInput = (input, callback) => { let width = input.offsetWidth; if (value) { - const testEl = strToEl(`${stripHTML(value)}`); + const testEl = strToEl(`${sanitise(value)}`); testEl.style.position = 'absolute'; testEl.style.padding = '0'; testEl.style.top = '-9999px'; @@ -203,8 +161,8 @@ export const calcWidthOfInput = (input, callback) => { }; export const sortByAlpha = (a, b) => { - const labelA = (a.label || a.value).toLowerCase(); - const labelB = (b.label || b.value).toLowerCase(); + const labelA = `${a.label || a.value}`.toLowerCase(); + const labelB = `${b.label || b.value}`.toLowerCase(); if (labelA < labelB) { return -1; @@ -241,15 +199,6 @@ export const getWindowHeight = () => { ); }; -export const reduceToValues = (items, key = 'value') => { - const values = items.reduce((prev, current) => { - prev.push(current[key]); - return prev; - }, []); - - return values; -}; - export const fetchFromObject = (object, path) => { const index = path.indexOf('.'); @@ -284,7 +233,5 @@ export const diff = (a, b) => { const aKeys = Object.keys(a).sort(); const bKeys = Object.keys(b).sort(); - return aKeys.filter((i) => { - return bKeys.indexOf(i) < 0; - }); -} + return aKeys.filter(i => bKeys.indexOf(i) < 0); +}; diff --git a/src/scripts/lib/utils.test.js b/src/scripts/lib/utils.test.js index 53f5889..182eb4a 100644 --- a/src/scripts/lib/utils.test.js +++ b/src/scripts/lib/utils.test.js @@ -1,14 +1,13 @@ import { expect } from 'chai'; import { stub } from 'sinon'; import { - reduceToValues, getRandomNumber, generateChars, generateId, getType, isType, isElement, - stripHTML, + sanitise, sortByAlpha, sortByScore, fetchFromObject, @@ -18,54 +17,6 @@ import { } from './utils'; describe('utils', () => { - describe('reduceToValues', () => { - const items = [ - { - id: 1, - choiceId: 1, - groupId: -1, - value: 'Item one', - label: 'Item one', - active: false, - highlighted: false, - customProperties: null, - placeholder: false, - keyCode: null, - }, - { - id: 2, - choiceId: 2, - groupId: -1, - value: 'Item two', - label: 'Item two', - active: true, - highlighted: false, - customProperties: null, - placeholder: false, - keyCode: null, - }, - { - id: 3, - choiceId: 3, - groupId: -1, - value: 'Item three', - label: 'Item three', - active: true, - highlighted: true, - customProperties: null, - placeholder: false, - keyCode: null, - }, - ]; - - it('returns an array of item values', () => { - const expectedResponse = [items[0].value, items[1].value, items[2].value]; - - const actualResponse = reduceToValues(items); - expect(actualResponse).to.eql(expectedResponse); - }); - }); - describe('getRandomNumber', () => { it('returns random number between range', () => { for (let index = 0; index < 10; index++) { @@ -154,10 +105,10 @@ describe('utils', () => { }); }); - describe('stripHTML', () => { + describe('sanitise', () => { it('strips HTML from value', () => { const value = ''; - const output = stripHTML(value); + const output = sanitise(value); expect(output).to.equal( '<script&rt;somethingMalicious();</script&rt;', ); @@ -247,25 +198,6 @@ describe('utils', () => { }); }); - describe('reduceToValues', () => { - it('reduces an array of objects to an array of values using given key', () => { - const values = [ - { name: 'The Strokes' }, - { name: 'Arctic Monkeys' }, - { name: 'Oasis' }, - { name: 'Tame Impala' }, - ]; - - const output = reduceToValues(values, 'name'); - expect(output).to.eql([ - 'The Strokes', - 'Arctic Monkeys', - 'Oasis', - 'Tame Impala', - ]); - }); - }); - describe('fetchFromObject', () => { it('fetches value from object using given path', () => { const object = {