From 0e17c598e1e82d40c5145e5135007bffe507200f Mon Sep 17 00:00:00 2001 From: ismay Date: Tue, 12 May 2020 14:35:44 +0200 Subject: [PATCH 1/2] fix(select): debounce menu width measurement --- packages/core/src/Select/Select.js | 14 ++++++-- .../Select/debounce/__tests__/index.test.js | 35 +++++++++++++++++++ packages/core/src/Select/debounce/index.js | 30 ++++++++++++++++ 3 files changed, 77 insertions(+), 2 deletions(-) create mode 100644 packages/core/src/Select/debounce/__tests__/index.test.js create mode 100644 packages/core/src/Select/debounce/index.js diff --git a/packages/core/src/Select/Select.js b/packages/core/src/Select/Select.js index d65c8f4936..031ae8e884 100644 --- a/packages/core/src/Select/Select.js +++ b/packages/core/src/Select/Select.js @@ -3,6 +3,7 @@ import propTypes from '@dhis2/prop-types' import { sharedPropTypes } from '@dhis2/ui-constants' import { InputWrapper } from './InputWrapper.js' import { MenuWrapper } from './MenuWrapper.js' +import { debounce } from './debounce' // Keycodes for the keypress event handlers const ESCAPE_KEY = 27 @@ -32,7 +33,16 @@ export class Select extends Component { window.removeEventListener('resize', this.setMenuWidth) } - setMenuWidth = () => { + /** + * We're debouncing this so it doesn't fire continually during a resize. + * + * Additionally we should use requestPostAnimationFrame to not trigger a forced + * layout, but that's just a proposal, and the added complexity of solving this + * in another manner does not seem worth it, considering the minor perf penalty. + * + * See: https://nolanlawson.com/2018/09/25/accurately-measuring-layout-on-the-web + */ + setMenuWidth = debounce(() => { const inputWidth = `${this.inputRef.current.offsetWidth}px` if (this.state.menuWidth !== inputWidth) { @@ -40,7 +50,7 @@ export class Select extends Component { menuWidth: inputWidth, }) } - } + }, 50) handleFocusInput = () => { this.inputRef.current.focus() diff --git a/packages/core/src/Select/debounce/__tests__/index.test.js b/packages/core/src/Select/debounce/__tests__/index.test.js new file mode 100644 index 0000000000..037d6b630d --- /dev/null +++ b/packages/core/src/Select/debounce/__tests__/index.test.js @@ -0,0 +1,35 @@ +import { debounce } from '../index.js' + +beforeEach(() => { + jest.useFakeTimers() +}) + +describe('debounce', () => { + it('should call the debounced function once after the timeout without immediate', () => { + const spy = jest.fn() + const debounced = debounce(spy, 100) + + debounced() + debounced() + + expect(spy).not.toHaveBeenCalled() + + jest.runAllTimers() + + expect(spy).toHaveBeenCalledTimes(1) + }) + + it('should call the debounced function once immediately if immediate is set', () => { + const spy = jest.fn() + const debounced = debounce(spy, 100, true) + + debounced() + debounced() + + expect(spy).toHaveBeenCalledTimes(1) + + jest.runAllTimers() + + expect(spy).toHaveBeenCalledTimes(1) + }) +}) diff --git a/packages/core/src/Select/debounce/index.js b/packages/core/src/Select/debounce/index.js new file mode 100644 index 0000000000..68247ab227 --- /dev/null +++ b/packages/core/src/Select/debounce/index.js @@ -0,0 +1,30 @@ +/** + * Returns a function, that, as long as it continues to be invoked, will not be triggered. The + * function will be called after it stops being called for N milliseconds. If `immediate` is + * passed, trigger the function on the leading edge, instead of the trailing. + */ + +export const debounce = (func, wait, immediate) => { + let timeout + + return (...args) => { + const context = this + + const later = () => { + timeout = null + + if (!immediate) { + func.apply(context, args) + } + } + + const callNow = immediate && !timeout + + clearTimeout(timeout) + timeout = setTimeout(later, wait) + + if (callNow) { + func.apply(context, args) + } + } +} From f355ea6889e7217f91bdbf7cfd932be2bc0063a8 Mon Sep 17 00:00:00 2001 From: ismay Date: Thu, 14 May 2020 17:27:59 +0200 Subject: [PATCH 2/2] test(select): use different test approach --- .../integration/MultiSelect/position/index.js | 158 ++++++++++-------- .../SingleSelect/position/index.js | 76 +++++++-- cypress/support/getAll.js | 9 + cypress/support/index.js | 1 + packages/core/src/Select/MenuWrapper.js | 2 +- 5 files changed, 160 insertions(+), 86 deletions(-) create mode 100644 cypress/support/getAll.js diff --git a/cypress/integration/MultiSelect/position/index.js b/cypress/integration/MultiSelect/position/index.js index bebc52c64e..ca6fa99d86 100644 --- a/cypress/integration/MultiSelect/position/index.js +++ b/cypress/integration/MultiSelect/position/index.js @@ -31,105 +31,129 @@ When('the window is scrolled down', () => { }) When('the window is resized to a greater width', () => { - waitForResizeObserver(() => { - cy.viewport(1200, 660) - }) + cy.viewport(1200, 660) }) When('an option is clicked', () => { - waitForResizeObserver(() => { - cy.contains('option one').click() - }) + cy.contains('option one').click() }) Then('the input is empty', () => { - cy.get('[data-test="dhis2-uicore-select-input"]').then($el => { - cy.wrap($el.outerHeight()).as('emptyInputHeight') + cy.get('[data-test="dhis2-uicore-select-input"]').then(inputs => { + expect(inputs.length).to.equal(1) + + const $input = inputs[0] + const inputRect = $input.getBoundingClientRect() + + cy.wrap(inputRect.height).as('emptyInputHeight') }) + cy.get('[data-test="dhis2-uicore-select-input"] .root').should('be.empty') }) Then('the Input grows in height', () => { - cy.get('@emptyInputHeight').then(emptyInputHeight => { - cy.get('[data-test="dhis2-uicore-select-input"]').then($el => { - expect($el.outerHeight()).to.be.greaterThan(emptyInputHeight) - }) - }) + const emptyInputHeight = '@emptyInputHeight' + const inputDataTest = '[data-test="dhis2-uicore-select-input"]' + + cy.getAll(emptyInputHeight, inputDataTest).should( + ([emptyInputHeight, inputs]) => { + expect(inputs.length).to.equal(1) + + const $input = inputs[0] + const inputRect = $input.getBoundingClientRect() + + expect(inputRect.height).to.be.greaterThan(emptyInputHeight) + } + ) }) Then('the top of the menu is aligned with the bottom of the input', () => { - // This test is used by the window scroll scenario - // so needs to take y into account for the anchor - getAnchorAndMenuRects().then(([anchorRect, menuRect]) => { - expect(menuRect.top).to.equal( - anchorRect.y - anchorRect.top + anchorRect.height - ) + const selectDataTest = '[data-test="dhis2-uicore-multiselect"]' + const menuDataTest = '[data-test="dhis2-uicore-select-menu-menuwrapper"]' + + cy.getAll(selectDataTest, menuDataTest).should(([selects, menus]) => { + expect(selects.length).to.equal(1) + expect(menus.length).to.equal(1) + + const $select = selects[0] + const $menu = menus[0] + + const selectRect = $select.getBoundingClientRect() + const menuRect = $menu.getBoundingClientRect() + + expect(menuRect.top).to.equal(selectRect.bottom) }) }) Then('the bottom of the menu is aligned with the top of the input', () => { - getAnchorAndMenuRects().then(([anchorRect, menuRect]) => { - expect(anchorRect.top).to.equal(menuRect.bottom) + const selectDataTest = '[data-test="dhis2-uicore-multiselect"]' + const menuDataTest = '[data-test="dhis2-uicore-select-menu-menuwrapper"]' + + cy.getAll(selectDataTest, menuDataTest).should(([selects, menus]) => { + expect(selects.length).to.equal(1) + expect(menus.length).to.equal(1) + + const $select = selects[0] + const $menu = menus[0] + + const selectRect = $select.getBoundingClientRect() + const menuRect = $menu.getBoundingClientRect() + + expect(selectRect.top).to.equal(menuRect.bottom) }) }) Then('it is rendered on top of the MultiSelect', () => { - getAnchorAndMenuRects().then(([anchorRect, menuRect]) => { - expect(anchorRect.top).to.be.greaterThan(menuRect.top) - expect(menuRect.bottom).to.be.greaterThan(anchorRect.bottom) + const selectDataTest = '[data-test="dhis2-uicore-multiselect"]' + const menuDataTest = '[data-test="dhis2-uicore-select-menu-menuwrapper"]' + + cy.getAll(selectDataTest, menuDataTest).should(([selects, menus]) => { + expect(selects.length).to.equal(1) + expect(menus.length).to.equal(1) + + const $select = selects[0] + const $menu = menus[0] + + const selectRect = $select.getBoundingClientRect() + const menuRect = $menu.getBoundingClientRect() + + expect(selectRect.top).to.be.greaterThan(menuRect.top) + expect(menuRect.bottom).to.be.greaterThan(selectRect.bottom) }) }) Then('the left of the Menu is aligned with the left of the Input', () => { - getAnchorAndMenuRects().then(([anchorRect, menuRect]) => { - expect(anchorRect.left).to.equal(menuRect.left) - }) -}) + const selectDataTest = '[data-test="dhis2-uicore-multiselect"]' + const menuDataTest = '[data-test="dhis2-uicore-select-menu-menuwrapper"]' -Then('the Menu and the Input have an equal width', () => { - cy.get('[data-test="dhis2-uicore-multiselect"] .root-input').then( - $input => { - cy.get('[data-test="dhis2-uicore-select-menu-menuwrapper"]').then( - $menu => { - expect($input.outerWidth()).to.equal($menu.outerWidth()) - } - ) - } - ) -}) + cy.getAll(selectDataTest, menuDataTest).should(([selects, menus]) => { + expect(selects.length).to.equal(1) + expect(menus.length).to.equal(1) -function getAnchorAndMenuRects() { - return cy.getPositionsBySelectors( - '[data-test="dhis2-uicore-multiselect"]', - '[data-test="dhis2-uicore-select-menu-menuwrapper"]' - ) -} + const $select = selects[0] + const $menu = menus[0] -function waitForResizeObserver(callback) { - cy.window().then(() => { - const id = 'resize-observer-callback-executed' - const oldNode = document.getElementById(id) + const selectRect = $select.getBoundingClientRect() + const menuRect = $menu.getBoundingClientRect() - // Cleanup - if (oldNode) { - oldNode.parentNode.removeChild(oldNode) - } + expect(selectRect.left).to.equal(menuRect.left) + }) +}) + +Then('the Menu and the Input have an equal width', () => { + const inputDataTest = '[data-test="dhis2-uicore-multiselect"] .root-input' + const menuDataTest = '[data-test="dhis2-uicore-select-menu-menuwrapper"]' - cy.get('[data-test="dhis2-uicore-select"]').then($el => { - const el = $el[0] - const observer = new ResizeObserver(() => { - // Create element to wait for when resizeObserver callback is executed - const newNode = document.createElement('div') - newNode.setAttribute('id', id) - el.parentNode.appendChild(newNode) - }) + cy.getAll(inputDataTest, menuDataTest).should(([inputs, menus]) => { + expect(inputs.length).to.equal(1) + expect(menus.length).to.equal(1) - observer.observe(el) + const $input = inputs[0] + const $menu = menus[0] - callback() + const inputRect = $input.getBoundingClientRect() + const menuRect = $menu.getBoundingClientRect() - // Wait for element and DOM redraw - return cy.get(`#${id}`).then(() => cy.wait(1)) - }) + expect(inputRect.width).to.equal(menuRect.width) }) -} +}) diff --git a/cypress/integration/SingleSelect/position/index.js b/cypress/integration/SingleSelect/position/index.js index 4a190d6e8b..19175d0cff 100644 --- a/cypress/integration/SingleSelect/position/index.js +++ b/cypress/integration/SingleSelect/position/index.js @@ -30,38 +30,78 @@ When('the window is scrolled down', () => { }) Then('the top of the menu is aligned with the bottom of the input', () => { - // This test is used by the window scroll scenario - // so needs to take y into account for the anchor - getAnchorAndMenuRects().then(([anchorRect, menuRect]) => { - expect(menuRect.top).to.equal(anchorRect.bottom - anchorRect.y) + const selectDataTest = '[data-test="dhis2-uicore-singleselect"]' + const menuDataTest = '[data-test="dhis2-uicore-select-menu-menuwrapper"]' + + cy.getAll(selectDataTest, menuDataTest).should(([selects, menus]) => { + expect(selects.length).to.equal(1) + expect(menus.length).to.equal(1) + + const $select = selects[0] + const $menu = menus[0] + + const selectRect = $select.getBoundingClientRect() + const menuRect = $menu.getBoundingClientRect() + + expect(menuRect.top).to.equal(selectRect.bottom) }) }) Then('the bottom of the menu is aligned with the top of the input', () => { - getAnchorAndMenuRects().then(([anchorRect, menuRect]) => { - expect(anchorRect.top).to.equal(menuRect.bottom) + const selectDataTest = '[data-test="dhis2-uicore-singleselect"]' + const menuDataTest = '[data-test="dhis2-uicore-select-menu-menuwrapper"]' + + cy.getAll(selectDataTest, menuDataTest).should(([selects, menus]) => { + expect(selects.length).to.equal(1) + expect(menus.length).to.equal(1) + + const $select = selects[0] + const $menu = menus[0] + + const selectRect = $select.getBoundingClientRect() + const menuRect = $menu.getBoundingClientRect() + + expect(selectRect.top).to.equal(menuRect.bottom) }) }) Then('it is rendered on top of the SingleSelect', () => { - getAnchorAndMenuRects().then(([anchorRect, menuRect]) => { - expect(anchorRect.top).to.be.greaterThan(menuRect.top) - expect(menuRect.bottom).to.be.greaterThan(anchorRect.bottom) + const selectDataTest = '[data-test="dhis2-uicore-singleselect"]' + const menuDataTest = '[data-test="dhis2-uicore-select-menu-menuwrapper"]' + + cy.getAll(selectDataTest, menuDataTest).should(([selects, menus]) => { + expect(selects.length).to.equal(1) + expect(menus.length).to.equal(1) + + const $select = selects[0] + const $menu = menus[0] + + const selectRect = $select.getBoundingClientRect() + const menuRect = $menu.getBoundingClientRect() + + expect(selectRect.top).to.be.greaterThan(menuRect.top) + expect(menuRect.bottom).to.be.greaterThan(selectRect.bottom) }) }) Then( 'the left of the SingleSelect is aligned with the left of the anchor', () => { - getAnchorAndMenuRects().then(([anchorRect, menuRect]) => { - expect(anchorRect.left).to.equal(menuRect.left) + const selectDataTest = '[data-test="dhis2-uicore-singleselect"]' + const menuDataTest = + '[data-test="dhis2-uicore-select-menu-menuwrapper"]' + + cy.getAll(selectDataTest, menuDataTest).should(([selects, menus]) => { + expect(selects.length).to.equal(1) + expect(menus.length).to.equal(1) + + const $select = selects[0] + const $menu = menus[0] + + const selectRect = $select.getBoundingClientRect() + const menuRect = $menu.getBoundingClientRect() + + expect(selectRect.left).to.equal(menuRect.left) }) } ) - -function getAnchorAndMenuRects() { - return cy.getPositionsBySelectors( - '[data-test="dhis2-uicore-singleselect"]', - '[data-test="dhis2-uicore-select-menu-menuwrapper"]' - ) -} diff --git a/cypress/support/getAll.js b/cypress/support/getAll.js new file mode 100644 index 0000000000..e829a044de --- /dev/null +++ b/cypress/support/getAll.js @@ -0,0 +1,9 @@ +Cypress.Commands.add('getAll', (...elements) => { + const promise = cy.wrap([], { log: false }) + + for (const element of elements) { + promise.then(arr => cy.get(element).then(got => cy.wrap([...arr, got]))) + } + + return promise +}) diff --git a/cypress/support/index.js b/cypress/support/index.js index 371b95b8da..a4f6c3231c 100644 --- a/cypress/support/index.js +++ b/cypress/support/index.js @@ -5,6 +5,7 @@ import './all' import './clickWith' import './find' import './get' +import './getAll' import './getPositionsBySelectors' import './uploadMultipleFiles' import './uploadSingleFile' diff --git a/packages/core/src/Select/MenuWrapper.js b/packages/core/src/Select/MenuWrapper.js index 7b19fb525d..d7243a7ead 100644 --- a/packages/core/src/Select/MenuWrapper.js +++ b/packages/core/src/Select/MenuWrapper.js @@ -22,7 +22,7 @@ const MenuWrapper = ({