Add new Modal Dialogue component #1668

colinrotherham · 2019-11-27T12:06:47Z

Afternoon 👋

As discussed with @hannalaakso, @timpaul, @amyhupe our team has been working on contributing a new "Modal dialogue" component to the Design System.

Sadly I'm going to be handing over work on this to @FeyAgape and @Ash-Wilson so this is still a draft for now, but more work to come!

Acceptance criteria

See alphagov/govuk-design-system-backlog#30 (comment)

Changes to GOV.UK Frontend

To support a modal component I've had to enhance GOV.UK Frontend slightly:

Adjusted iframe previews for position: fixed content
Adjusted iframe previews for custom variant styling (fixed height for modals)
Add support for Nunjucks macro call blocks {% call macro() %}{% endcall %}

But here's a draft pull request where we're up to anyway.

colinrotherham · 2019-11-27T12:07:13Z

Can we please have this on a Heroku preview URL so I can share with the team?

Thanks

edit: Preview URL

colinrotherham · 2019-11-27T12:49:13Z

Also any design feedback would be appreciated.

We're aware we added another embedded "crown" SVG, so you might want a better way of adding this to the page without duplicating the SVG markup.

I added the crown as it's already in use here:
https://components.publishing.service.gov.uk/component-guide/modal_dialogue

But not sure what the user need is.

soniaturcotte · 2019-11-27T13:48:21Z

@alex-ju and I worked on one here, not sure if you've seen it:
https://content-publisher.publishing.service.gov.uk/component-guide/modal_dialogue

colinrotherham · 2019-11-27T14:11:26Z

@alex-ju and I worked on one here, not sure if you've seen it:
https://content-publisher.publishing.service.gov.uk/component-guide/modal_dialogue

Ahh is this the same code as used on as https://components.publishing.service.gov.uk/component-guide/modal_dialogue? If you're the original designer thank you 😁

We've also improved the focus trapping so if you manage to click out of the modal, you're pulled back in again, not just looping from last/first items.

We've added onOpen and onClose callbacks, added native HTML5 <dialog> support, and turned it into a Nunjucks macro.

It's been great to improve one that's already in production.

NickColley · 2019-11-27T15:54:21Z

I have some questions on the design, maybe these have been decided as part of research...

what is the user need for the crown logo on this modal?
we try to avoid using icons instead of words if possible as it is less accessible to use icons on their own, has a word label been considered here?
is it clear that the close button is a button? should it have more visual affordance?

colinrotherham · 2019-11-28T09:48:00Z

@NickColley Maybe @soniaturcotte can help?

We used the design from https://components.publishing.service.gov.uk/component-guide/modal_dialogue and it tested well when we did user research.

colinrotherham · 2019-11-28T14:21:56Z

I've removed the vertical alignment fallback for older browsers using display: table-cell as it doesn't cater for overflowing content at all.

In IE10+ we've got basic flexbox support so that's good enough.

Older browsers like IE8 + IE9 will have the modal at the top of the screen.

colinrotherham · 2019-11-28T15:46:57Z

@NickColley Did you mean like this for the close button text?

Maybe it's something @FeyAgape can explore?

You're in her capable hands now 😊

hannalaakso

This is looking good, thanks Colin 🙌 A couple of small suggestions from me. I've only really had a chance to review app/ part which @colinrotherham was keen to get some feedback on, will look at the src/ files next.

app/assets/scss/partials/_app.scss

src/govuk/components/modal-dialogue/_modal-dialogue.scss

app/assets/scss/partials/_app.scss

src/govuk/components/modal-dialogue/modal-dialogue.yaml

app/app.js

app/views/layouts/iframe.njk

Caused by `.govuk-main-wrapper--auto-spacing`

Requires fixed height as iFrameResizer can’t handle `position: fixed` child content

NickColley · 2019-11-29T14:19:15Z

src/govuk/components/modal-dialogue/modal-dialogue.js

+  this.hasNativeDialog = 'showModal' in this.$dialogBox
+
+  // Allowed focussable elements
+  this.focussable = [


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.

Yeah sounds like a good enhancement.

You'd have to be very careful and keep the modal outside govuk-width-container or whatever the page wrapper element was.

<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 [inert] wrapper.

The early prototype version of the modal that GOV.UK based theirs on used both the inert attribute and aria-hidden to make the <main> wrapper non-actionable and this tested well with screen reader users. The modal markup was outside <main> and below <body>.

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.

NickColley · 2019-11-29T14:22:42Z

src/govuk/components/modal-dialogue/modal-dialogue.js

+ModalDialogue.prototype.init = function (options) {
+  this.options = options || {}
+
+  this.open = this.handleOpen.bind(this)


I wonder if these method binds should be in the constructor instead?

@NickColley I try to defer anything that uses more CPU cycles until it's actually needed (like when .init() gets run) but it's probably negligible so don't mind.

Thanks Colin 👍 We believe that the performance impact is negligible so we should make the change for consistency with other components.

NickColley · 2019-11-29T14:26:45Z

src/govuk/components/modal-dialogue/modal-dialogue.js

+  this.$lastActiveElement.focus()
+
+  // Optional 'onClose' callback
+  if (typeof this.options.onClose === 'function') {


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

@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 } }) }

hannalaakso · 2019-12-09T14:02:02Z

src/govuk/components/modal-dialogue/_modal-dialogue.scss

+@import "../../helpers/all";
+
+@include govuk-exports("govuk/component/modal-dialogue") {
+  $govuk-dialogue-width: 640px;


We probably want to use !default here so that the value can overridden.

hannalaakso · 2019-12-09T14:21:31Z

src/govuk/components/modal-dialogue/_modal-dialogue.scss

+    box-sizing: border-box;
+    display: block;
+    position: relative;
+    z-index: 1;


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.

I've made this 1001 so it is above the backdrop which also needs the z-index increasing.

Suggested change

z-index: 1;

z-index: 1001;

hannalaakso · 2019-12-09T14:28:14Z

src/govuk/components/modal-dialogue/_modal-dialogue.scss

+    width: 90%;
+    margin: auto;
+    padding: 0;
+    overflow-y: auto;


This might be okay as it is but just for reference, I notice that the GOV.UK Publishing example currently sets these to:

overflow-x: hidden; overflow-y: scroll;

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.

hannalaakso · 2019-12-09T14:34:11Z

src/govuk/components/modal-dialogue/_modal-dialogue.scss

+    background: govuk-colour("black");
+    pointer-events: none;
+    touch-action: none;
+  }


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.

hannalaakso · 2019-12-09T14:37:29Z

src/govuk/components/modal-dialogue/_modal-dialogue.scss

+    }
+  }
+
+  // New dialogue width, inline button + link


I'm not sure why these styles are set separately from the other ones in this file. Should this be a modifier class?

hannalaakso · 2019-12-09T14:37:37Z

src/govuk/components/modal-dialogue/_modal-dialogue.scss

+  }
+
+  // Fixed width
+  @include govuk-media-query($from: desktop) {


I'm not sure why these styles are set separately from the other ones in this file. Should this be a modifier class?

hannalaakso · 2019-12-09T14:54:41Z

src/govuk/components/modal-dialogue/modal-dialogue.js

+
+  // Close dialogue on close button click
+  this.$buttonClose.addEventListener('click', this.close)
+}


Not a big deal but I wonder if the events inside this block could live inside init(), it'll be tidier once we move the variable definitions to the constructor.

hannalaakso · 2019-12-09T14:55:13Z

src/govuk/components/modal-dialogue/modal-dialogue.js

+  this.$lastActiveElement = document.activeElement
+
+  // Disable scrolling, show wrapper
+  this.$container.classList.add('govuk-!-scroll-disabled')


Does govuk-!-scroll-disabled do anything? I can't see any styles created for it.

hannalaakso · 2019-12-09T17:45:23Z

src/govuk/components/modal-dialogue/template.njk

+          type: "button",
+          classes: "govuk-modal-dialogue__close",
+          attributes: {
+            "aria-label": "Close modal dialogue"


I wonder if all users will know what is meant by "modal dialogue" here? The users of GOV.UK Publisher components (which this is based on) would mainly be repeat users with at least a moderate level of computer literacy and are therefore not representative of the wider population at whole. If we could explore an alternative wording that would be great.

hannalaakso · 2019-12-10T16:39:26Z

src/govuk/components/modal-dialogue/modal-dialogue.js

+}
+
+// Ensure focus stays within modal
+ModalDialogue.prototype.handleFocusTrap = function (event) {


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.

To add to this point, what if a user navigates via landmarks? What is the impact on them when the modal is open.

hannalaakso · 2019-12-10T16:43:09Z

src/govuk/components/modal-dialogue/modal-dialogue.js

+    : this.$dialogBox.removeAttribute('open')
+
+  // Hide wrapper, enable scrolling
+  this.$module.classList.remove('govuk-modal-dialogue--open')


Could we save class names as variables in the constructor and reference them from there?

hannalaakso · 2019-12-10T16:52:55Z

src/govuk/components/modal-dialogue/_modal-dialogue.scss

+  }
+
+  // New dialogue width, inline button + link
+  @include govuk-media-query($from: tablet) {


Is the mobile/tablet experience meant to be similar to the GOV.UK Publishing modal?

This might be something for @Ash-Wilson to consider.

hannalaakso · 2019-12-10T16:55:06Z

src/govuk/components/modal-dialogue/modal-dialogue.js

+
+// Lock scroll, focus modal
+ModalDialogue.prototype.handleFocus = function () {
+  this.$dialogBox.scrollIntoView()


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?

hannalaakso · 2019-12-10T17:03:35Z

src/govuk/components/modal-dialogue/template.njk

+          viewbox="0 0 132 97"
+          height="30"
+          width="36"
+        >


Had a chat with @FeyAgape. We should consider removing the crown logo as there is no clear user need. Its usage here is not consistent with our other components and it adds complexity.

Its usage in public facing sites in this manner might also be problematic with regards to the branding guidelines; the Service Manual guidance mentions the use of the crown logo only in the header.

hannalaakso · 2019-12-10T17:05:21Z

src/govuk/components/modal-dialogue/template.njk

+        </svg>
+
+        {{ govukButton({
+          text: "×",


Had a chat with @FeyAgape. If possible, we should explore using a text equivalent here as icons might not be understood by all users.

hannalaakso · 2019-12-10T17:08:50Z

src/govuk/components/modal-dialogue/modal-dialogue.yaml

+previewLayout: modal
+
+examples:
+  - name: default


Could we make this example open the modal with a button? Similar to the GOV.UK Publishing modal example. This would allow us to test the transition of the modal being closed to it being open, and how the main page content is made actionable / non-actionable.

Doing this might require some tweaking of the review app.

hannalaakso · 2019-12-10T17:11:39Z

src/govuk/components/modal-dialogue/modal-dialogue.yaml

+      description:
+        html: <p class="govuk-body">Prompt text for the modal dialogue component goes here</p>
+
+  - name: open with scrolling content


Could we make this example to have some page content in the main page area? Similar to the GOV.UK Publishing modal example. This would allow us to test that the background is not scrollable / actionable when the modal is open.

hannalaakso · 2019-12-10T17:16:55Z

src/govuk/components/modal-dialogue/modal-dialogue.yaml

+- name: open
+  type: boolean
+  required: true
+  description: If true, modal dialogue element will be visible.


Should we consider calling the component "modal dialog" instead?

We're using the HTML <dialog> element and the industry docs talk about "modal dialog".

Trends over time of different names:
https://trends.google.com/trends/explore?cat=422&q=modal,dialog,modal%20dialog,modal%20dialogue,dialogue

NickColley · 2019-12-10T18:42:47Z

@hannalaakso I can help review this when you're happy later on, perhaps before it goes to the working group. I think we should do some real device testing at some point too.

nogginbox · 2020-06-11T10:23:38Z

What is the status of this PR? @hannalaakso, @NickColley is this still being worked on.

We're adding a timeout modal in our next sprint and I would like to use this component.

hannalaakso · 2020-06-11T16:59:00Z

@nogginbox Thanks for reaching out. This work depends on #1708 especially, and also #1722. It's unfortunately not possible to give a timeline on the work at the moment but we will pick it up when we're able to prioritise it.

nogginbox · 2020-07-03T08:40:27Z

src/govuk/components/modal-dialogue/_modal-dialogue.scss

+  .govuk-modal-dialogue,
+  .govuk-modal-dialogue__backdrop {
+    position: fixed;
+    z-index: 0;


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

z-index: 0;

z-index: 1000;

nogginbox · 2020-07-03T09:18:44Z

src/govuk/components/modal-dialogue/_modal-dialogue.scss

+
+  // Inner content
+  .govuk-modal-dialogue__content {
+    @include govuk-font($size: 16);


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-font($size: 16);

zaizous · 2020-08-14T17:08:04Z

Hi @hannalaakso @NickColley, do you guys have any update on the timeline?
We are looking into implementing time out in the coming sprints and would be good to make use of the GDS pattern.

hannalaakso · 2020-08-18T17:27:58Z

@zaizous No update at the moment. You might find the acceptance criteria which this component will need to meet helpful if you're researching this topic.

hannalaakso · 2021-03-01T13:27:58Z

I’m closing this PR as it’s currently blocked by #1722 and #1708.

Note for teams who are researching modal dialogues: the work done in this PR could be used as the basis for creating a modal dialogue. We would encourage teams to review:

acceptance criteria related to this PR, especially the points about design and accessibility
conversations on the modal dialogue backlog issue

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1668 November 27, 2019 12:11 Inactive

colinrotherham force-pushed the modal-dialogue branch from aab4515 to 49a0e1f Compare November 27, 2019 12:33

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1668 November 27, 2019 12:34 Inactive

colinrotherham changed the title ~~Modal dialogue~~ Add new Modal dialogue component Nov 27, 2019

colinrotherham changed the title ~~Add new Modal dialogue component~~ Add new Modal Dialogue component Nov 27, 2019

colinrotherham force-pushed the modal-dialogue branch from 49a0e1f to b77bf86 Compare November 28, 2019 14:20

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1668 November 28, 2019 14:21 Inactive

colinrotherham force-pushed the modal-dialogue branch from b77bf86 to 0913734 Compare November 28, 2019 14:42

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1668 November 28, 2019 14:42 Inactive

hannalaakso reviewed Nov 28, 2019

View reviewed changes

colinrotherham added 4 commits November 29, 2019 12:29

Fix increased margin above/below component preview

7be5dcf

Caused by `.govuk-main-wrapper--auto-spacing`

Add support for Nunjucks macro call blocks

aae7ff1

Add fixed-height “modal” preview template

956d5ae

Requires fixed height as iFrameResizer can’t handle `position: fixed` child content

Fix full-width layout in IE8

e346314

colinrotherham force-pushed the modal-dialogue branch from 0913734 to 125474b Compare November 29, 2019 12:38

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1668 November 29, 2019 12:38 Inactive

Add modal dialogue component

aba92ba

colinrotherham force-pushed the modal-dialogue branch from 125474b to aba92ba Compare November 29, 2019 12:43

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1668 November 29, 2019 12:43 Inactive

NickColley reviewed Nov 29, 2019

View reviewed changes

hannalaakso reviewed Dec 9, 2019

View reviewed changes

hannalaakso reviewed Dec 10, 2019

View reviewed changes

hannalaakso self-requested a review December 10, 2019 17:41

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1668 April 21, 2020 09:17 Inactive

nogginbox reviewed Jul 3, 2020

View reviewed changes

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1668 September 10, 2020 15:21 Inactive

govuk-design-system-ci deployed to govuk-frontend-review-pr-1668 October 12, 2020 17:59 View deployment

govuk-design-system-ci temporarily deployed to govuk-fronte-modal-dial-qqwhwc November 30, 2020 13:43 Inactive

hannalaakso closed this Mar 1, 2021

hannalaakso mentioned this pull request Oct 19, 2021

Modal dialogue alphagov/govuk-design-system-backlog#30

Open

Add new Modal Dialogue component #1668

Add new Modal Dialogue component #1668

Conversation

colinrotherham commented Nov 27, 2019 • edited by hannalaakso Loading

Acceptance criteria

Changes to GOV.UK Frontend

colinrotherham commented Nov 27, 2019 • edited by hannalaakso Loading

colinrotherham commented Nov 27, 2019

soniaturcotte commented Nov 27, 2019

colinrotherham commented Nov 27, 2019

NickColley commented Nov 27, 2019 • edited Loading

colinrotherham commented Nov 28, 2019 • edited Loading

colinrotherham commented Nov 28, 2019

colinrotherham commented Nov 28, 2019

hannalaakso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickColley Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannalaakso Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannalaakso Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

hannalaakso Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannalaakso Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

hannalaakso Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannalaakso Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannalaakso Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

NickColley Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

NickColley commented Dec 10, 2019 • edited Loading

nogginbox commented Jun 11, 2020 • edited Loading

hannalaakso commented Jun 11, 2020

nogginbox Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaizous commented Aug 14, 2020 • edited Loading

hannalaakso commented Aug 18, 2020

hannalaakso commented Mar 1, 2021

colinrotherham commented Nov 27, 2019 •

edited by hannalaakso

Loading

colinrotherham commented Nov 27, 2019 •

edited by hannalaakso

Loading

NickColley commented Nov 27, 2019 •

edited

Loading

colinrotherham commented Nov 28, 2019 •

edited

Loading

NickColley Nov 29, 2019 •

edited

Loading

hannalaakso Dec 9, 2019 •

edited

Loading

hannalaakso Dec 9, 2019 •

edited

Loading

hannalaakso Dec 10, 2019 •

edited

Loading

hannalaakso Dec 10, 2019 •

edited

Loading

hannalaakso Dec 10, 2019 •

edited

Loading

hannalaakso Dec 10, 2019 •

edited

Loading

hannalaakso Dec 10, 2019 •

edited

Loading

NickColley Dec 10, 2019 •

edited

Loading

NickColley commented Dec 10, 2019 •

edited

Loading

nogginbox commented Jun 11, 2020 •

edited

Loading

nogginbox Jul 3, 2020 •

edited

Loading

zaizous commented Aug 14, 2020 •

edited

Loading