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 eacc8420bce0..05b0adcd26e1 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 1c8f53ebd625..3241a71327e1 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 8f501d8113e2..3eb5891f8f85 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 e440573829fd..fb46578393e5 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 0dd6b1d454cb..5ee211c73b9e 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 590b5795c76e..0997ae7c205d 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 712185784145..5275f1a55670 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 a7c1c28982ef..b4010e0e0e25 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(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.isElement(fakejQuery)).toEqual(el.nodeType) + expect(Util.getElement(fakejQueryObject)).toEqual(el) }) })