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.

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

Add default Modal component #181

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add default Modal component #181

wants to merge 1 commit into from

Conversation

LucasBgt
Copy link
Contributor

@LucasBgt LucasBgt commented Aug 2, 2024

A default Modal component to create generic modal or to extend for specific purposes.

Javascript

It requires focus-trap dependency to be able to isolate the focus within the modal. This benefits from all the advantages of this library, as the clickOutsideDeactivates option or the many callbacks.

Basic modal
You can create a new basic modal using the data-module-modal attribute on the modal itself. This makes the default modal easier to create. If you have multiple basic modals, you'll need to name these in order to use the togglers correctly. To do so, use the data-modal-name attribute.

Specific modal
You can also create a specific modal with custom code by extending the Modal Class. To do so, you'll have to match the data-attribute with your extended Class.
Example: If you create a VideoModal.js with custom code to handle the video player, the HTML attribute must be: data-module-video-modal. Nothing new here, just using Modular.js.

Togglers
No need to create a dedicated Class for togglers, as they are just stored using the data-attribute data-[module-name]-toggler on HTML elements.

  • In the case of one basic modal, you only need to add data-modal-toggler on every toggler you want, including the close button inside the modal.
  • In the case of named basic modals (2 or more), you need to specify this name in the data-attribute. This will allow to toggle the correct modal and not all the modals in the page, since these are using the same default Class.
  • In the case of a specific modal, you need to match the data-attribute with your className. For the VideoModal example, it would be data-video-modal-toggler

Focus Trap Targets
The targets works exactly like the togglers. You can also set multiple HTML elements, or className (string), or whatever FocusTrap supports (see documentation). If not specified, it will use the modal itself.

CSS

Very basic css, the minimum to be able to test the modal without adding anything.

Copy link

vercel bot commented Aug 2, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
locomotive-boilerplate ✅ Ready (Inspect) Visit Preview Aug 2, 2024 5:24pm

@mcaskill
Copy link
Member

Since my proposal for a modal in 2021, browser support for <dialog> has improved greatly. I would recommend pivoting to using <dialog> and its DOM API or at least renaming adopting "dialog" (or "overlay") as the preferred term for this component/pattern. We should be more mindful of when to call an overlayed box a "modal" or not:

[…] Modal dialog boxes interrupt interaction with the rest of the page being inert, while non-modal dialog boxes allow interaction with the rest of the page.

Comment on lines +11 to +24
/**
* Creates a new Modal.
*
* @param {object} options - The module options.
* @param {string} options.dataName - The module data attribute name.
* @throws {TypeError} If the class does not have an active CSS class defined.
*/

static CLASS = {
EL: 'is-open',
HTML: 'has-modal-open',
}

constructor(options) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* Creates a new Modal.
*
* @param {object} options - The module options.
* @param {string} options.dataName - The module data attribute name.
* @throws {TypeError} If the class does not have an active CSS class defined.
*/
static CLASS = {
EL: 'is-open',
HTML: 'has-modal-open',
}
constructor(options) {
static CLASS = {
EL: 'is-open',
HTML: 'has-modal-open',
}
/**
* Creates a new Modal.
*
* @param {object} options - The module options.
* @param {string} options.dataName - The module data attribute name.
*/
constructor(options) {

Misplaced block comment and no related error to the @throws.

Comment on lines +19 to +22
static CLASS = {
EL: 'is-open',
HTML: 'has-modal-open',
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static CLASS = {
EL: 'is-open',
HTML: 'has-modal-open',
}
static OPEN_CLASS = {
EL: 'is-open',
HTML: 'has-modal-open',
}

CLASS is too generic and meaningless given its very specific use case.

I recommend OPEN_CLASS or something to that effect.


// Data
this.moduleName = options.name
this.dataName = this.getData('name') || options.dataName
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.dataName = this.getData('name') || options.dataName
this.dataName = options.dataName || this.getData('name')

I think the constructor's option.dataName parameter should have precedence over the options.el's attribute given the class' purpose is to decorate the base element.

Comment on lines +36 to +112
// UI
this.$togglers = document.querySelectorAll(`[data-${this.dataName}-toggler]`)
this.$focusTrapTargets = Array.from(this.el.querySelectorAll(`[data-${this.dataName}-target]`))

// Focus trap options
this.focusTrapOptions = {
/**
* There is a delay between when the class is applied
* and when the element is focusable
*/
checkCanFocusTrap: (trapContainers) => {
const results = trapContainers.map((trapContainer) => {
return new Promise((resolve) => {
const interval = setInterval(() => {
if (
getComputedStyle(trapContainer).visibility !==
'hidden'
) {
resolve()
clearInterval(interval)
}
}, 5)
})
})

// Return a promise that resolves when all the trap containers are able to receive focus
return Promise.all(results)
},

onActivate: () => {
this.el.classList.add(Modal.CLASS.EL)
$html.classList.add(Modal.CLASS.HTML)
$html.classList.add('has-'+this.dataName+'-open')
this.el.setAttribute('aria-hidden', false)
this.isOpen = true

this.onActivate?.();
},

onPostActivate: () => {
this.$togglers.forEach(($toggler) => {
$toggler.setAttribute('aria-expanded', true)
})
},

onDeactivate: () => {
this.el.classList.remove(Modal.CLASS.EL)
$html.classList.remove(Modal.CLASS.HTML)
$html.classList.remove('has-'+this.dataName+'-open')
this.el.setAttribute('aria-hidden', true)
this.isOpen = false

this.onDeactivate?.();
},

onPostDeactivate: () => {
this.$togglers.forEach(($toggler) => {
$toggler.setAttribute('aria-expanded', false)
})
},

clickOutsideDeactivates: true,
}

this.isOpen = false
}

/////////////////
// Lifecycle
/////////////////
init() {
this.onBeforeInit?.()

this.focusTrap = createFocusTrap(
this.$focusTrapTargets.length > 0 ? this.$focusTrapTargets : [this.el],
this.focusTrapOptions
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// UI
this.$togglers = document.querySelectorAll(`[data-${this.dataName}-toggler]`)
this.$focusTrapTargets = Array.from(this.el.querySelectorAll(`[data-${this.dataName}-target]`))
// Focus trap options
this.focusTrapOptions = {
/**
* There is a delay between when the class is applied
* and when the element is focusable
*/
checkCanFocusTrap: (trapContainers) => {
const results = trapContainers.map((trapContainer) => {
return new Promise((resolve) => {
const interval = setInterval(() => {
if (
getComputedStyle(trapContainer).visibility !==
'hidden'
) {
resolve()
clearInterval(interval)
}
}, 5)
})
})
// Return a promise that resolves when all the trap containers are able to receive focus
return Promise.all(results)
},
onActivate: () => {
this.el.classList.add(Modal.CLASS.EL)
$html.classList.add(Modal.CLASS.HTML)
$html.classList.add('has-'+this.dataName+'-open')
this.el.setAttribute('aria-hidden', false)
this.isOpen = true
this.onActivate?.();
},
onPostActivate: () => {
this.$togglers.forEach(($toggler) => {
$toggler.setAttribute('aria-expanded', true)
})
},
onDeactivate: () => {
this.el.classList.remove(Modal.CLASS.EL)
$html.classList.remove(Modal.CLASS.HTML)
$html.classList.remove('has-'+this.dataName+'-open')
this.el.setAttribute('aria-hidden', true)
this.isOpen = false
this.onDeactivate?.();
},
onPostDeactivate: () => {
this.$togglers.forEach(($toggler) => {
$toggler.setAttribute('aria-expanded', false)
})
},
clickOutsideDeactivates: true,
}
this.isOpen = false
}
/////////////////
// Lifecycle
/////////////////
init() {
this.onBeforeInit?.()
this.focusTrap = createFocusTrap(
this.$focusTrapTargets.length > 0 ? this.$focusTrapTargets : [this.el],
this.focusTrapOptions
)
// UI
this.$togglers = document.querySelectorAll(`[data-${this.dataName}-toggler]`)
this.$focusTrapTargets = Array.from(this.el.querySelectorAll(`[data-${this.dataName}-target]`))
this.isOpen = false
}
getFocusTrapOptions() {
return {
/**
* There is a delay between when the class is applied
* and when the element is focusable
*/
checkCanFocusTrap: (trapContainers) => {
const results = trapContainers.map((trapContainer) => {
return new Promise((resolve) => {
const interval = setInterval(() => {
if (
getComputedStyle(trapContainer).visibility !==
'hidden'
) {
resolve()
clearInterval(interval)
}
}, 5)
})
})
// Return a promise that resolves when all the trap containers are able to receive focus
return Promise.all(results)
},
onActivate: () => {
this.el.classList.add(Modal.CLASS.EL)
$html.classList.add(Modal.CLASS.HTML)
$html.classList.add('has-'+this.dataName+'-open')
this.el.setAttribute('aria-hidden', false)
this.isOpen = true
this.onActivate?.();
},
onPostActivate: () => {
this.$togglers.forEach(($toggler) => {
$toggler.setAttribute('aria-expanded', true)
})
},
onDeactivate: () => {
this.el.classList.remove(Modal.CLASS.EL)
$html.classList.remove(Modal.CLASS.HTML)
$html.classList.remove('has-'+this.dataName+'-open')
this.el.setAttribute('aria-hidden', true)
this.isOpen = false
this.onDeactivate?.();
},
onPostDeactivate: () => {
this.$togglers.forEach(($toggler) => {
$toggler.setAttribute('aria-expanded', false)
})
},
clickOutsideDeactivates: true,
}
}
/////////////////
// Lifecycle
/////////////////
init() {
this.onBeforeInit?.()
this.focusTrap = createFocusTrap(
this.$focusTrapTargets.length > 0 ? this.$focusTrapTargets : [this.el],
this.getFocusTrapOptions()
)

this.focusTrapOptions should be moved to a separate method, something like this.getFocusTrapOptions which would be easier to override/extend in a sub-class.

}

open(args) {
if (this.isOpen) return
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (this.isOpen) return
if (this.isOpen) {
return
}

The compiler is going to minify this anyway, might as well maximize readability.

}

close(args) {
if (!this.isOpen) return
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!this.isOpen) return
if (!this.isOpen) {
return
}

The compiler is going to minify this anyway, might as well maximize readability.

Comment on lines +6 to +9
/**
* Generic component to display a modal.
*
*/
Copy link
Member

Choose a reason for hiding this comment

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

Please provide an example(s) of the HTML needed to use this module as well as information on how to sub-class this base class, including which hooks are available (such as this.onBeforeInit).

@@ -46,6 +46,8 @@ const CSS_CLASS = Object.freeze({
// Custom js events
const CUSTOM_EVENT = Object.freeze({
RESIZE_END: 'loco.resizeEnd',
VISIT_START: 'visit.start',
MODAL_OPEN: 'modal.open',
Copy link
Member

Choose a reason for hiding this comment

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

There should be an equivalent MODAL_CLOSE custom event.

@@ -14,6 +15,12 @@ export default class extends module {
}
});

load.on('loading', (transition, oldContainer) => {
const args = { transition, oldContainer };
// Dispatch custom event
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Dispatch custom event

Useless comment.

@@ -46,6 +46,8 @@ const CSS_CLASS = Object.freeze({
// Custom js events
const CUSTOM_EVENT = Object.freeze({
RESIZE_END: 'loco.resizeEnd',
VISIT_START: 'visit.start',
Copy link
Member

Choose a reason for hiding this comment

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

I assume VISIT_START is named this way based on SWUP's visit:start hook?

Visit started: transition to a new page begins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants