From 6e1c9096f08ed21063fd6ba16aa998a3ac4149f9 Mon Sep 17 00:00:00 2001 From: GeoSot Date: Thu, 13 May 2021 18:17:20 +0300 Subject: [PATCH] Move get element functionality to a helper (#33327) Looking around on js components I found out many checks, different expressed but with same purpose. Some of them are trying to parse string to element, others, jQuery element to js simple nodeElement etc With this Pr, I am trying to give a standard way to parse an element So this pr: * Creates `getElement` helper that tries to parse an argument to element or null * Changes `isElement` to make explicit checks and return Boolean * fixes tests deficiencies --- js/src/base-component.js | 3 +- js/src/collapse.js | 11 ++----- js/src/dropdown.js | 8 ++--- js/src/tooltip.js | 27 ++++------------- js/src/util/index.js | 27 ++++++++++++++++- js/tests/unit/collapse.spec.js | 3 +- js/tests/unit/dropdown.spec.js | 3 +- js/tests/unit/util/index.spec.js | 52 ++++++++++++++++++++++++++------ 8 files changed, 85 insertions(+), 49 deletions(-) diff --git a/js/src/base-component.js b/js/src/base-component.js index eacc8420b..05b0adcd2 100644 --- a/js/src/base-component.js +++ b/js/src/base-component.js @@ -9,6 +9,7 @@ import Data from './dom/data' import { emulateTransitionEnd, execute, + getElement, getTransitionDurationFromElement } from './util/index' import EventHandler from './dom/event-handler' @@ -23,7 +24,7 @@ const VERSION = '5.0.0' class BaseComponent { constructor(element) { - element = typeof element === 'string' ? document.querySelector(element) : element + element = getElement(element) if (!element) { return diff --git a/js/src/collapse.js b/js/src/collapse.js index 1c8f53ebd..3241a7132 100644 --- a/js/src/collapse.js +++ b/js/src/collapse.js @@ -7,9 +7,9 @@ import { defineJQueryPlugin, + getElement, getSelectorFromElement, getElementFromSelector, - isElement, reflow, typeCheckConfig } from './util/index' @@ -272,14 +272,7 @@ class Collapse extends BaseComponent { _getParent() { let { parent } = this._config - if (isElement(parent)) { - // it's a jQuery object - if (typeof parent.jquery !== 'undefined' || typeof parent[0] !== 'undefined') { - parent = parent[0] - } - } else { - parent = SelectorEngine.findOne(parent) - } + parent = getElement(parent) const selector = `${SELECTOR_DATA_TOGGLE}[data-bs-parent="${parent}"]` diff --git a/js/src/dropdown.js b/js/src/dropdown.js index 8f501d811..3eb5891f8 100644 --- a/js/src/dropdown.js +++ b/js/src/dropdown.js @@ -9,6 +9,7 @@ import * as Popper from '@popperjs/core' import { defineJQueryPlugin, + getElement, getElementFromSelector, isDisabled, isElement, @@ -166,12 +167,7 @@ class Dropdown extends BaseComponent { if (this._config.reference === 'parent') { referenceElement = parent } else if (isElement(this._config.reference)) { - referenceElement = this._config.reference - - // Check if it's jQuery element - if (typeof this._config.reference.jquery !== 'undefined') { - referenceElement = this._config.reference[0] - } + referenceElement = getElement(this._config.reference) } else if (typeof this._config.reference === 'object') { referenceElement = this._config.reference } diff --git a/js/src/tooltip.js b/js/src/tooltip.js index e44057382..fb4657839 100644 --- a/js/src/tooltip.js +++ b/js/src/tooltip.js @@ -10,6 +10,7 @@ import * as Popper from '@popperjs/core' import { defineJQueryPlugin, findShadowRoot, + getElement, getUID, isElement, isRTL, @@ -256,7 +257,7 @@ class Tooltip extends BaseComponent { const attachment = this._getAttachment(placement) this._addAttachmentClass(attachment) - const container = this._getContainer() + const { container } = this._config Data.set(tip, this.constructor.DATA_KEY, this) if (!this._element.ownerDocument.documentElement.contains(this.tip)) { @@ -385,10 +386,8 @@ class Tooltip extends BaseComponent { return } - if (typeof content === 'object' && isElement(content)) { - if (content.jquery) { - content = content[0] - } + if (isElement(content)) { + content = getElement(content) // content is a DOM node or a jQuery if (this._config.html) { @@ -518,18 +517,6 @@ class Tooltip extends BaseComponent { this.getTipElement().classList.add(`${CLASS_PREFIX}-${this.updateAttachment(attachment)}`) } - _getContainer() { - if (this._config.container === false) { - return document.body - } - - if (isElement(this._config.container)) { - return this._config.container - } - - return SelectorEngine.findOne(this._config.container) - } - _getAttachment(placement) { return AttachmentMap[placement.toUpperCase()] } @@ -664,16 +651,14 @@ class Tooltip extends BaseComponent { } }) - if (config && typeof config.container === 'object' && config.container.jquery) { - config.container = config.container[0] - } - config = { ...this.constructor.Default, ...dataAttributes, ...(typeof config === 'object' && config ? config : {}) } + config.container = config.container === false ? document.body : getElement(config.container) + if (typeof config.delay === 'number') { config.delay = { show: config.delay, diff --git a/js/src/util/index.js b/js/src/util/index.js index 0dd6b1d45..5ee211c73 100644 --- a/js/src/util/index.js +++ b/js/src/util/index.js @@ -1,3 +1,5 @@ +import SelectorEngine from '../dom/selector-engine' + /** * -------------------------------------------------------------------------- * Bootstrap (v5.0.0): util/index.js @@ -100,7 +102,29 @@ const triggerTransitionEnd = element => { element.dispatchEvent(new Event(TRANSITION_END)) } -const isElement = obj => (obj[0] || obj).nodeType +const isElement = obj => { + if (!obj || typeof obj !== 'object') { + return false + } + + if (typeof obj.jquery !== 'undefined') { + obj = obj[0] + } + + return typeof obj.nodeType !== 'undefined' +} + +const getElement = obj => { + if (isElement(obj)) { // it's a jQuery object or a node element + return obj.jquery ? obj[0] : obj + } + + if (typeof obj === 'string' && obj.length > 0) { + return SelectorEngine.findOne(obj) + } + + return null +} const emulateTransitionEnd = (element, duration) => { let called = false @@ -238,6 +262,7 @@ const execute = callback => { } export { + getElement, getUID, getSelectorFromElement, getElementFromSelector, diff --git a/js/tests/unit/collapse.spec.js b/js/tests/unit/collapse.spec.js index 590b5795c..0997ae7c2 100644 --- a/js/tests/unit/collapse.spec.js +++ b/js/tests/unit/collapse.spec.js @@ -58,7 +58,8 @@ describe('Collapse', () => { const collapseEl = fixtureEl.querySelector('div.collapse') const myCollapseEl = fixtureEl.querySelector('.my-collapse') const fakejQueryObject = { - 0: myCollapseEl + 0: myCollapseEl, + jquery: 'foo' } const collapse = new Collapse(collapseEl, { parent: fakejQueryObject diff --git a/js/tests/unit/dropdown.spec.js b/js/tests/unit/dropdown.spec.js index 712185784..5275f1a55 100644 --- a/js/tests/unit/dropdown.spec.js +++ b/js/tests/unit/dropdown.spec.js @@ -3,7 +3,7 @@ import EventHandler from '../../src/dom/event-handler' import { noop } from '../../src/util' /** Test helpers */ -import { getFixture, clearFixture, createEvent, jQueryMock } from '../helpers/fixture' +import { clearFixture, createEvent, getFixture, jQueryMock } from '../helpers/fixture' describe('Dropdown', () => { let fixtureEl @@ -467,6 +467,7 @@ describe('Dropdown', () => { const btnDropdown = fixtureEl.querySelector('[data-bs-toggle="dropdown"]') const virtualElement = { + nodeType: 1, getBoundingClientRect() { return { width: 0, diff --git a/js/tests/unit/util/index.spec.js b/js/tests/unit/util/index.spec.js index a7c1c2898..b4010e0e0 100644 --- a/js/tests/unit/util/index.spec.js +++ b/js/tests/unit/util/index.spec.js @@ -1,7 +1,7 @@ import * as Util from '../../../src/util/index' /** Test helpers */ -import { getFixture, clearFixture } from '../../helpers/fixture' +import { clearFixture, getFixture } from '../../helpers/fixture' describe('Util', () => { let fixtureEl @@ -171,24 +171,58 @@ describe('Util', () => { }) describe('isElement', () => { - it('should detect if the parameter is an element or not', () => { - fixtureEl.innerHTML = '
' + it('should detect if the parameter is an element or not and return Boolean', () => { + fixtureEl.innerHTML = + [ + '
', + '
' + ].join('') - const el = document.querySelector('div') + const el = fixtureEl.querySelector('#foo') - expect(Util.isElement(el)).toEqual(el.nodeType) - expect(Util.isElement({})).toEqual(undefined) + expect(Util.isElement(el)).toEqual(true) + expect(Util.isElement({})).toEqual(false) + expect(Util.isElement(fixtureEl.querySelectorAll('.test'))).toEqual(false) }) it('should detect jQuery element', () => { fixtureEl.innerHTML = '
' - const el = document.querySelector('div') + const el = fixtureEl.querySelector('div') const fakejQuery = { - 0: el + 0: el, + jquery: 'foo' } - expect(Util.isElement(fakejQuery)).toEqual(el.nodeType) + expect(Util.isElement(fakejQuery)).toEqual(true) + }) + }) + + describe('getElement', () => { + it('should try to parse element', () => { + fixtureEl.innerHTML = + [ + '
', + '
' + ].join('') + + const el = fixtureEl.querySelector('div') + + expect(Util.getElement(el)).toEqual(el) + expect(Util.getElement('#foo')).toEqual(el) + expect(Util.getElement('#fail')).toBeNull() + expect(Util.getElement({})).toBeNull() + expect(Util.getElement([])).toBeNull() + expect(Util.getElement()).toBeNull() + expect(Util.getElement(null)).toBeNull() + expect(Util.getElement(fixtureEl.querySelectorAll('.test'))).toBeNull() + + const fakejQueryObject = { + 0: el, + jquery: 'foo' + } + + expect(Util.getElement(fakejQueryObject)).toEqual(el) }) })