Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept data-bs-body option in the configuration object as well #33248

Merged
merged 3 commits into from
Mar 16, 2021
Merged

Accept data-bs-body option in the configuration object as well #33248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f56bd5
Accept data-bs-body option in the configuration object as well
GeoSot Mar 2, 2021
1b35d63
Fix Markdown table formatting and tweak the wording on backdrop
mdo Mar 15, 2021
9f7cd1f
Merge branch 'main' into use-config-obj-on-offcanvas
XhmikosR Mar 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 55 additions & 21 deletions js/src/offcanvas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,16 @@ import {
getElementFromSelector,
getSelectorFromElement,
getTransitionDurationFromElement,
isVisible
isDisabled,
isVisible,
typeCheckConfig
} from './util/index'
import { hide as scrollBarHide, reset as scrollBarReset } from './util/scrollbar'
import Data from './dom/data'
import EventHandler from './dom/event-handler'
import BaseComponent from './base-component'
import SelectorEngine from './dom/selector-engine'
import Manipulator from './dom/manipulator'

/**
* ------------------------------------------------------------------------
Expand All @@ -29,10 +32,20 @@ const DATA_KEY = 'bs.offcanvas'
const EVENT_KEY = `.${DATA_KEY}`
const DATA_API_KEY = '.data-api'
const ESCAPE_KEY = 'Escape'
const DATA_BODY_ACTIONS = 'data-bs-body'

const Default = {
backdrop: true,
keyboard: true,
scroll: false
}

const DefaultType = {
backdrop: 'boolean',
keyboard: 'boolean',
scroll: 'boolean'
}

const CLASS_NAME_BACKDROP_BODY = 'offcanvas-backdrop'
const CLASS_NAME_DISABLED = 'disabled'
const CLASS_NAME_SHOW = 'show'
const CLASS_NAME_TOGGLING = 'offcanvas-toggling'
const ACTIVE_SELECTOR = `.offcanvas.show, .${CLASS_NAME_TOGGLING}`
Expand All @@ -55,14 +68,24 @@ const SELECTOR_DATA_TOGGLE = '[data-bs-toggle="offcanvas"]'
*/

class Offcanvas extends BaseComponent {
constructor(element) {
constructor(element, config) {
super(element)

this._config = this._getConfig(config)
this._isShown = element.classList.contains(CLASS_NAME_SHOW)
this._bodyOptions = element.getAttribute(DATA_BODY_ACTIONS) || ''
this._addEventListeners()
}

// Getters

static get Default() {
return Default
}

static get DATA_KEY() {
return DATA_KEY
}

// Public

toggle(relatedTarget) {
Expand All @@ -83,11 +106,11 @@ class Offcanvas extends BaseComponent {
this._isShown = true
this._element.style.visibility = 'visible'

if (this._bodyOptionsHas('backdrop') || !this._bodyOptions.length) {
if (this._config.backdrop) {
document.body.classList.add(CLASS_NAME_BACKDROP_BODY)
}

if (!this._bodyOptionsHas('scroll')) {
if (!this._config.scroll) {
scrollBarHide()
}

Expand Down Expand Up @@ -129,11 +152,11 @@ class Offcanvas extends BaseComponent {
this._element.removeAttribute('role')
this._element.style.visibility = 'hidden'

if (this._bodyOptionsHas('backdrop') || !this._bodyOptions.length) {
if (this._config.backdrop) {
document.body.classList.remove(CLASS_NAME_BACKDROP_BODY)
}

if (!this._bodyOptionsHas('scroll')) {
if (!this._config.scroll) {
scrollBarReset()
}

Expand All @@ -144,6 +167,18 @@ class Offcanvas extends BaseComponent {
setTimeout(completeCallback, getTransitionDurationFromElement(this._element))
}

// Private

_getConfig(config) {
config = {
...Default,
...Manipulator.getDataAttributes(this._element),
...(typeof config === 'object' ? config : {})
}
typeCheckConfig(NAME, config, DefaultType)
return config
}

_enforceFocusOnElement(element) {
EventHandler.off(document, EVENT_FOCUSIN) // guard against infinite focus loop
EventHandler.on(document, EVENT_FOCUSIN, event => {
Expand All @@ -156,15 +191,11 @@ class Offcanvas extends BaseComponent {
element.focus()
}

_bodyOptionsHas(option) {
return this._bodyOptions.split(',').includes(option)
}

_addEventListeners() {
EventHandler.on(this._element, EVENT_CLICK_DISMISS, SELECTOR_DATA_DISMISS, () => this.hide())

EventHandler.on(document, 'keydown', event => {
if (event.key === ESCAPE_KEY) {
if (this._config.keyboard && event.key === ESCAPE_KEY) {
this.hide()
}
})
Expand All @@ -181,15 +212,17 @@ class Offcanvas extends BaseComponent {

static jQueryInterface(config) {
return this.each(function () {
const data = Data.get(this, DATA_KEY) || new Offcanvas(this)
const data = Data.get(this, DATA_KEY) || new Offcanvas(this, typeof config === 'object' ? config : {})

if (typeof config === 'string') {
if (typeof data[config] === 'undefined') {
throw new TypeError(`No method named "${config}"`)
}
if (typeof config !== 'string') {
return
}

data[config](this)
if (data[config] === undefined || config.startsWith('_') || config === 'constructor') {
throw new TypeError(`No method named "${config}"`)
}

data[config](this)
GeoSot marked this conversation as resolved.
Show resolved Hide resolved
})
}
}
Expand All @@ -207,7 +240,7 @@ EventHandler.on(document, EVENT_CLICK_DATA_API, SELECTOR_DATA_TOGGLE, function (
event.preventDefault()
}

if (this.disabled || this.classList.contains(CLASS_NAME_DISABLED)) {
if (isDisabled(this)) {
return
}

Expand All @@ -225,6 +258,7 @@ EventHandler.on(document, EVENT_CLICK_DATA_API, SELECTOR_DATA_TOGGLE, function (
}

const data = Data.get(target, DATA_KEY) || new Offcanvas(target)

data.toggle(this)
})

Expand Down
17 changes: 17 additions & 0 deletions js/src/util/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,22 @@ const isVisible = element => {
return false
}

const isDisabled = element => {
if (!element || element.nodeType !== Node.ELEMENT_NODE) {
return true
}

if (element.classList.contains('disabled')) {
return true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd return false here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure. In code if an element not exists, you don't want to count it as valid. right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the check is for isDisabled. If the element doesn't exist then it can't be disabled?

Copy link
Member Author

@GeoSot GeoSot Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we check if a button/anchor/input (or another div) is disabled. Almost in any case it will not be null. The null check is for the 'just in case' situation
I can omitted if you find it more accurate

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer being stricter :) That being said, the logic seems off to me for the aforementioned reason.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another proposal is to throw an exception on line 157.
No-one has checked all these before, so if there is any other opinion I would like to know in order to finalize this

@twbs/js-review

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, if the DOM element does not exist it should not be considered as enabled. The absence of the DOM element is also like the disability 🤔

}

if (typeof element.disabled !== 'undefined') {
return element.disabled
}

return element.getAttribute('disabled') !== 'false'
}

const findShadowRoot = element => {
if (!document.documentElement.attachShadow) {
return null
Expand Down Expand Up @@ -226,6 +242,7 @@ export {
emulateTransitionEnd,
typeCheckConfig,
isVisible,
isDisabled,
findShadowRoot,
noop,
reflow,
Expand Down
134 changes: 133 additions & 1 deletion js/tests/unit/offcanvas.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import Offcanvas from '../../src/offcanvas'
import EventHandler from '../../src/dom/event-handler'

/** Test helpers */
import { clearFixture, getFixture, jQueryMock, createEvent } from '../helpers/fixture'
import { clearFixture, createEvent, getFixture, jQueryMock } from '../helpers/fixture'

describe('Offcanvas', () => {
let fixtureEl
Expand All @@ -22,6 +22,18 @@ describe('Offcanvas', () => {
})
})

describe('Default', () => {
it('should return plugin default config', () => {
expect(Offcanvas.Default).toEqual(jasmine.any(Object))
})
})

describe('DATA_KEY', () => {
it('should return plugin data key', () => {
expect(Offcanvas.DATA_KEY).toEqual('bs.offcanvas')
})
})

describe('constructor', () => {
it('should call hide when a element with data-bs-dismiss="offcanvas" is clicked', () => {
fixtureEl.innerHTML = [
Expand Down Expand Up @@ -70,6 +82,68 @@ describe('Offcanvas', () => {

expect(offCanvas.hide).not.toHaveBeenCalled()
})

it('should not hide if esc is pressed but with keyboard = false', () => {
fixtureEl.innerHTML = '<div class="offcanvas"></div>'

const offCanvasEl = fixtureEl.querySelector('.offcanvas')
const offCanvas = new Offcanvas(offCanvasEl, { keyboard: false })
const keyDownEsc = createEvent('keydown')
keyDownEsc.key = 'Escape'

spyOn(offCanvas, 'hide')

document.dispatchEvent(keyDownEsc)

expect(offCanvas.hide).not.toHaveBeenCalled()
})
})

describe('config', () => {
it('should have default values', () => {
fixtureEl.innerHTML = [
'<div class="offcanvas">',
'</div>'
].join('')

const offCanvasEl = fixtureEl.querySelector('.offcanvas')
const offCanvas = new Offcanvas(offCanvasEl)

expect(offCanvas._config.backdrop).toEqual(true)
expect(offCanvas._config.keyboard).toEqual(true)
expect(offCanvas._config.scroll).toEqual(false)
GeoSot marked this conversation as resolved.
Show resolved Hide resolved
})

it('should read data attributes and override default config', () => {
fixtureEl.innerHTML = [
'<div class="offcanvas" data-bs-scroll="true" data-bs-backdrop="false" data-bs-keyboard="false">',
'</div>'
].join('')

const offCanvasEl = fixtureEl.querySelector('.offcanvas')
const offCanvas = new Offcanvas(offCanvasEl)

expect(offCanvas._config.backdrop).toEqual(false)
expect(offCanvas._config.keyboard).toEqual(false)
expect(offCanvas._config.scroll).toEqual(true)
})

it('given a config object must override data attributes', () => {
fixtureEl.innerHTML = [
'<div class="offcanvas" data-bs-scroll="true" data-bs-backdrop="false" data-bs-keyboard="false">',
'</div>'
].join('')

const offCanvasEl = fixtureEl.querySelector('.offcanvas')
const offCanvas = new Offcanvas(offCanvasEl, {
backdrop: true,
keyboard: true,
scroll: false
})
expect(offCanvas._config.backdrop).toEqual(true)
expect(offCanvas._config.keyboard).toEqual(true)
expect(offCanvas._config.scroll).toEqual(false)
})
})

describe('toggle', () => {
Expand Down Expand Up @@ -280,6 +354,64 @@ describe('Offcanvas', () => {
jQueryMock.fn.offcanvas = Offcanvas.jQueryInterface
jQueryMock.elements = [div]

expect(() => {
jQueryMock.fn.offcanvas.call(jQueryMock, action)
}).toThrowError(TypeError, `No method named "${action}"`)
})

it('should throw error on protected method', () => {
fixtureEl.innerHTML = '<div></div>'

const div = fixtureEl.querySelector('div')
const action = '_getConfig'

jQueryMock.fn.offcanvas = Offcanvas.jQueryInterface
jQueryMock.elements = [div]

expect(() => {
jQueryMock.fn.offcanvas.call(jQueryMock, action)
}).toThrowError(TypeError, `No method named "${action}"`)
})

it('should throw error if method "constructor" is being called', () => {
fixtureEl.innerHTML = '<div></div>'

const div = fixtureEl.querySelector('div')
const action = 'constructor'

jQueryMock.fn.offcanvas = Offcanvas.jQueryInterface
jQueryMock.elements = [div]

expect(() => {
jQueryMock.fn.offcanvas.call(jQueryMock, action)
}).toThrowError(TypeError, `No method named "${action}"`)
})

it('should throw error on protected method', () => {
fixtureEl.innerHTML = '<div></div>'

const div = fixtureEl.querySelector('div')
const action = '_getConfig'

jQueryMock.fn.offcanvas = Offcanvas.jQueryInterface
jQueryMock.elements = [div]

try {
jQueryMock.fn.offcanvas.call(jQueryMock, action)
} catch (error) {
expect(error.message).toEqual(`No method named "${action}"`)
}
GeoSot marked this conversation as resolved.
Show resolved Hide resolved
})

it('should throw error if method "constructor" is being called', () => {
fixtureEl.innerHTML = '<div></div>'

const div = fixtureEl.querySelector('div')
const action = 'constructor'

jQueryMock.fn.offcanvas = Offcanvas.jQueryInterface
jQueryMock.elements = [div]

try {
jQueryMock.fn.offcanvas.call(jQueryMock, action)
} catch (error) {
Expand Down
Loading