-
Notifications
You must be signed in to change notification settings - Fork 339
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 new Modal Dialogue component #1668
Changes from 1 commit
7be5dcf
aae7ff1
956d5ae
e346314
aba92ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,161 @@ | ||||||
@import "../../settings/all"; | ||||||
@import "../../tools/all"; | ||||||
@import "../../helpers/all"; | ||||||
|
||||||
@include govuk-exports("govuk/component/modal-dialogue") { | ||||||
$govuk-dialogue-width: 640px; | ||||||
|
||||||
.govuk-modal-dialogue, | ||||||
.govuk-modal-dialogue__backdrop { | ||||||
position: fixed; | ||||||
z-index: 0; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Increase to 1000 so that this will appear above all other elements already in the site. I had to do this when I've used this code as several elements floated on top of the backdrop.
Suggested change
|
||||||
top: 0; | ||||||
left: 0; | ||||||
width: 100%; | ||||||
height: 100%; | ||||||
} | ||||||
|
||||||
// Hide dialogue when closed | ||||||
.govuk-modal-dialogue { | ||||||
display: none; | ||||||
} | ||||||
|
||||||
// Show dialogue when opened | ||||||
.govuk-modal-dialogue--open { | ||||||
display: block; | ||||||
} | ||||||
|
||||||
// Wrapper to handle overflow scrolling | ||||||
.govuk-modal-dialogue__wrapper { | ||||||
box-sizing: border-box; | ||||||
display: flex; | ||||||
height: 100%; | ||||||
@include govuk-responsive-padding(7, "top"); | ||||||
@include govuk-responsive-padding(7, "bottom"); | ||||||
overflow-y: auto; | ||||||
align-items: flex-start; // sass-lint:disable no-duplicate-properties | ||||||
align-items: safe center; | ||||||
} | ||||||
|
||||||
// HTML5 dialogue component | ||||||
.govuk-modal-dialogue__box { | ||||||
box-sizing: border-box; | ||||||
display: block; | ||||||
position: relative; | ||||||
z-index: 1; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might want to increase this, just in the (non-ideal) case that the page has other competing elements. I notice that the GOV.UK Publishing equivalent sets this to 1000. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've made this 1001 so it is above the backdrop which also needs the z-index increasing.
Suggested change
|
||||||
width: 90%; | ||||||
margin: auto; | ||||||
padding: 0; | ||||||
overflow-y: auto; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might be okay as it is but just for reference, I notice that the GOV.UK Publishing example currently sets these to:
I don't think we currently have an example with page content in the main area so we need to add one (see #1668 (comment)) in order to check that the main page content is not scrollable and the modal is. |
||||||
border: $govuk-focus-width solid govuk-colour("black"); | ||||||
background: govuk-colour("white"); | ||||||
|
||||||
// Add focus outline to dialogue | ||||||
&:focus { | ||||||
outline: $govuk-focus-width solid $govuk-focus-colour; | ||||||
} | ||||||
|
||||||
// Hide browser backdrop | ||||||
&::backdrop { | ||||||
display: none; | ||||||
} | ||||||
} | ||||||
|
||||||
// Header with close button | ||||||
.govuk-modal-dialogue__header { | ||||||
@include govuk-clearfix; | ||||||
@include govuk-responsive-margin(5, "bottom"); | ||||||
padding-bottom: $govuk-focus-width; | ||||||
color: govuk-colour("white"); | ||||||
background-color: govuk-colour("black"); | ||||||
text-align: right; | ||||||
} | ||||||
|
||||||
// Inner content | ||||||
.govuk-modal-dialogue__content { | ||||||
@include govuk-font($size: 16); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was able to safely remove this font definition as I wanted the site's standard fonts to be used. We're a non gov body so can't use the standard GDS font and have overrides in place to not use them. This line being included broke our GDS font overrides.
Suggested change
|
||||||
@include govuk-responsive-padding(6); | ||||||
padding-top: 0; | ||||||
} | ||||||
|
||||||
.govuk-modal-dialogue__description { | ||||||
@include govuk-responsive-margin(4, "bottom"); | ||||||
} | ||||||
|
||||||
// Remove bottom margins | ||||||
.govuk-modal-dialogue__description:last-child, | ||||||
.govuk-modal-dialogue__description > :last-child, | ||||||
.govuk-modal-dialogue__content > :last-child { | ||||||
margin-bottom: 0; | ||||||
} | ||||||
|
||||||
// Custom backdrop | ||||||
.govuk-modal-dialogue__backdrop { | ||||||
opacity: .8; | ||||||
background: govuk-colour("black"); | ||||||
pointer-events: none; | ||||||
touch-action: none; | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this block should live below https://github.com/alphagov/govuk-frontend/pull/1668/files#diff-40be96fa6566e06e9d9d9ab4d1d3d831R8 to in order to keep the backdrop styles together. |
||||||
|
||||||
// Crown icon | ||||||
.govuk-modal-dialogue__crown { | ||||||
display: block; | ||||||
margin: 6px 0 0 6px; | ||||||
@include govuk-responsive-margin(5, "left"); | ||||||
float: left; | ||||||
} | ||||||
|
||||||
// Heading | ||||||
.govuk-modal-dialogue__heading:last-child { | ||||||
margin-bottom: 0; | ||||||
} | ||||||
|
||||||
// Close button | ||||||
.govuk-modal-dialogue__close { | ||||||
$font-size: 36px; | ||||||
$line-height: 1; | ||||||
|
||||||
display: block; | ||||||
width: auto; | ||||||
min-width: 44px; | ||||||
margin: 0; | ||||||
padding: 2px 5px; | ||||||
float: right; | ||||||
color: govuk-colour("white"); | ||||||
background-color: govuk-colour("black"); | ||||||
box-shadow: none !important; | ||||||
font-size: $font-size; | ||||||
@if $govuk-typography-use-rem { | ||||||
font-size: govuk-px-to-rem($font-size); | ||||||
} | ||||||
@include govuk-typography-weight-bold; | ||||||
line-height: $line-height; | ||||||
|
||||||
&:hover { | ||||||
color: govuk-colour("black"); | ||||||
background-color: govuk-colour("yellow"); | ||||||
} | ||||||
|
||||||
&:active { | ||||||
top: 0; | ||||||
} | ||||||
} | ||||||
|
||||||
// New dialogue width, inline button + link | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why these styles are set separately from the other ones in this file. Should this be a modifier class? |
||||||
@include govuk-media-query($from: tablet) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the mobile/tablet experience meant to be similar to the GOV.UK Publishing modal? This might be something for @Ash-Wilson to consider. |
||||||
.govuk-modal-dialogue__content { | ||||||
padding-top: 0; | ||||||
} | ||||||
|
||||||
.govuk-modal-dialogue__box { | ||||||
width: percentage($govuk-dialogue-width / map-get($govuk-breakpoints, desktop)); | ||||||
} | ||||||
} | ||||||
|
||||||
// Fixed width | ||||||
@include govuk-media-query($from: desktop) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why these styles are set separately from the other ones in this file. Should this be a modifier class? |
||||||
.govuk-modal-dialogue__box { | ||||||
width: $govuk-dialogue-width; | ||||||
} | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{% macro govukModalDialogue(params) %} | ||
{%- include "./template.njk" -%} | ||
{% endmacro %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,186 @@ | ||
import { nodeListForEach } from '../../common' | ||
import '../../vendor/polyfills/Element/prototype/classList' | ||
import '../../vendor/polyfills/Function/prototype/bind' | ||
import '../../vendor/polyfills/Event' // addEventListener and event.target normaliziation | ||
|
||
function ModalDialogue ($module) { | ||
this.$module = $module | ||
this.$dialogBox = $module.querySelector('dialog') | ||
this.$container = document.documentElement | ||
|
||
// Check for browser support | ||
this.hasNativeDialog = 'showModal' in this.$dialogBox | ||
|
||
// Allowed focussable elements | ||
this.focussable = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is an 'inert' feature we could consider referring to when reviewing the focus trap functionality in this proposal: https://github.com/WICG/inert It ships in chromium, I'm not sure if it's a proper standard yet but worth thinking about. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah sounds like a good enhancement. You'd have to be very careful and keep the modal outside <body>
<div class="govuk-width-container" inert>…</div>
<div data-module="govuk-modal-dialogue">…</div>
</body> It could go horribly wrong if the modal was inside the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The early prototype version of the modal that GOV.UK based theirs on used both the Colin does have a valid point: we need to be clear in the guidance about where the modal markup is placed in the page content. |
||
'button', | ||
'[href]', | ||
'input', | ||
'select', | ||
'textarea' | ||
] | ||
} | ||
|
||
// Initialize component | ||
ModalDialogue.prototype.init = function (options) { | ||
this.options = options || {} | ||
|
||
this.open = this.handleOpen.bind(this) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if these method binds should be in the constructor instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @NickColley I try to defer anything that uses more CPU cycles until it's actually needed (like when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks Colin 👍 We believe that the performance impact is negligible so we should make the change for consistency with other components. |
||
this.close = this.handleClose.bind(this) | ||
this.focus = this.handleFocus.bind(this) | ||
this.focusTrap = this.handleFocusTrap.bind(this) | ||
this.boundKeyDown = this.handleKeyDown.bind(this) | ||
|
||
// Modal elements | ||
this.$buttonClose = this.$dialogBox.querySelector('.govuk-modal-dialogue__close') | ||
this.$focussable = this.$dialogBox.querySelectorAll(this.focussable.toString()) | ||
this.$focusableLast = this.$focussable[this.$focussable.length - 1] | ||
this.$focusElement = this.options.focusElement || this.$dialogBox | ||
|
||
if (this.$dialogBox.hasAttribute('open')) { | ||
this.open() | ||
} | ||
|
||
this.initEvents() | ||
|
||
return this | ||
} | ||
|
||
// Initialize component events | ||
ModalDialogue.prototype.initEvents = function (options) { | ||
if (this.options.triggerElement) { | ||
this.options.triggerElement.addEventListener('click', this.open) | ||
} | ||
|
||
// Close dialogue on close button click | ||
this.$buttonClose.addEventListener('click', this.close) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a big deal but I wonder if the events inside this block could live inside |
||
|
||
// Open modal | ||
ModalDialogue.prototype.handleOpen = function (event) { | ||
if (event) { | ||
event.preventDefault() | ||
} | ||
|
||
// Save last-focussed element | ||
this.$lastActiveElement = document.activeElement | ||
|
||
// Disable scrolling, show wrapper | ||
this.$container.classList.add('govuk-!-scroll-disabled') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does |
||
this.$module.classList.add('govuk-modal-dialogue--open') | ||
|
||
// Close on escape key, trap focus | ||
document.addEventListener('keydown', this.boundKeyDown, true) | ||
|
||
// Optional 'onOpen' callback | ||
if (typeof this.options.onOpen === 'function') { | ||
this.options.onOpen.call(this) | ||
} | ||
|
||
// Skip open if already open | ||
if (this.$dialogBox.hasAttribute('open')) { | ||
return | ||
} | ||
|
||
// Show modal | ||
this.hasNativeDialog | ||
? this.$dialogBox.show() | ||
: this.$dialogBox.setAttribute('open', '') | ||
|
||
// Handle focus | ||
this.focus() | ||
} | ||
|
||
// Close modal | ||
ModalDialogue.prototype.handleClose = function (event) { | ||
if (event) { | ||
event.preventDefault() | ||
} | ||
|
||
// Skip close if already closed | ||
if (!this.$dialogBox.hasAttribute('open')) { | ||
return | ||
} | ||
|
||
// Hide modal | ||
this.hasNativeDialog | ||
? this.$dialogBox.close() | ||
: this.$dialogBox.removeAttribute('open') | ||
|
||
// Hide wrapper, enable scrolling | ||
this.$module.classList.remove('govuk-modal-dialogue--open') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we save class names as variables in the constructor and reference them from there? |
||
this.$container.classList.remove('govuk-!-scroll-disabled') | ||
|
||
// Restore focus to last active element | ||
this.$lastActiveElement.focus() | ||
|
||
// Optional 'onClose' callback | ||
if (typeof this.options.onClose === 'function') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we need to consider how we're documenting the JavaScript options for components, since we use data-attributes elsewhere instead of programmatic options. See also: Proposal programmatic and data attribute api interface for components There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @NickColley Yeah good point. Here's how we currently use this Modal API on our prototype: var $dialogue = document.querySelector('[data-module="govuk-modal-dialogue"]')
var $dialogueButtonResume = document.querySelector('.govuk-modal-dialogue__continue')
if ($dialogue && $dialogueButtonResume) {
var modal = new window.ModalDialogue($dialogue)
.init({
focusElement: $dialogueButtonResume,
onClose: function () {
// Extend session timeout here
}
})
} |
||
this.options.onClose.call(this) | ||
} | ||
|
||
// Remove escape key and trap focus listener | ||
document.removeEventListener('keydown', this.boundKeyDown, true) | ||
} | ||
|
||
// Lock scroll, focus modal | ||
ModalDialogue.prototype.handleFocus = function () { | ||
this.$dialogBox.scrollIntoView() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the component is going to follow the mobile experience of the GOV.UK Publisher modal (see #1668 (comment)) then this might not be needed, since the modal will either have a fixed position or fill the viewport? |
||
this.$focusElement.focus({ preventScroll: true }) | ||
} | ||
|
||
// Ensure focus stays within modal | ||
ModalDialogue.prototype.handleFocusTrap = function (event) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good to understand more about the scenario in which focus might escape the modal. This function executes when the tab key is pressed, but the background should already be non-actionable so that none of the background elements can be tabbed to. Fixing #1668 (comment) should help with this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To add to this point, what if a user navigates via landmarks? What is the impact on them when the modal is open. |
||
var $focusElement | ||
|
||
// Check for tabbing outside dialog | ||
var hasFocusEscaped = document.activeElement !== this.$dialogBox | ||
|
||
// Loop inner focussable elements | ||
if (hasFocusEscaped) { | ||
nodeListForEach(this.$focussable, function (element) { | ||
// Actually, focus is on an inner focussable element | ||
if (hasFocusEscaped && document.activeElement === element) { | ||
hasFocusEscaped = false | ||
} | ||
}) | ||
|
||
// Wrap focus back to first element | ||
$focusElement = hasFocusEscaped | ||
? this.$dialogBox | ||
: undefined | ||
} | ||
|
||
// Wrap focus back to first/last element | ||
if (!$focusElement) { | ||
if ((document.activeElement === this.$focusableLast && !event.shiftKey) || !this.$focussable.length) { | ||
$focusElement = this.$dialogBox | ||
} else if (document.activeElement === this.$dialogBox && event.shiftKey) { | ||
$focusElement = this.$focusableLast | ||
} | ||
} | ||
|
||
// Wrap focus | ||
if ($focusElement) { | ||
event.preventDefault() | ||
$focusElement.focus({ preventScroll: true }) | ||
} | ||
} | ||
|
||
// Listen for key presses | ||
ModalDialogue.prototype.handleKeyDown = function (event) { | ||
var KEY_TAB = 9 | ||
var KEY_ESCAPE = 27 | ||
|
||
switch (event.keyCode) { | ||
case KEY_TAB: | ||
this.focusTrap(event) | ||
break | ||
|
||
case KEY_ESCAPE: | ||
this.close() | ||
break | ||
} | ||
} | ||
|
||
export default ModalDialogue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to use
!default
here so that the value can overridden.