Skip to content

Commit

Permalink
feat: Dialog and Dialog-fullscreen: Make title-text required (#4999)
Browse files Browse the repository at this point in the history
  • Loading branch information
margaree authored Sep 23, 2024
1 parent f474abf commit b5adc2b
Show file tree
Hide file tree
Showing 7 changed files with 37 additions and 14 deletions.
2 changes: 1 addition & 1 deletion components/dialog/demo/dialog-confirm.html
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ <h2>Confirm Dialog</h2>
<d2l-demo-snippet>
<template>
<d2l-button id="openConfirm">Show Confirm</d2l-button>
<d2l-dialog-confirm id="confirm" title-text="Confirm Title" text="Are you sure you want more cookies?">
<d2l-dialog-confirm id="confirm" text="Are you sure you want more cookies?">
<d2l-button slot="footer" primary data-dialog-action="ok">Yes</d2l-button>
<d2l-button slot="footer" data-dialog-action>No</d2l-button>
</d2l-dialog-confirm>
Expand Down
7 changes: 6 additions & 1 deletion components/dialog/dialog-fullscreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { DialogMixin } from './dialog-mixin.js';
import { dialogStyles } from './dialog-styles.js';
import { getUniqueId } from '../../helpers/uniqueId.js';
import { LocalizeCoreElement } from '../../helpers/localize-core-element.js';
import { PropertyRequiredMixin } from '../../mixins/property-required/property-required-mixin.js';
import { styleMap } from 'lit/directives/style-map.js';

const mediaQueryList = window.matchMedia('(max-width: 615px), (max-height: 420px) and (max-width: 900px)');
Expand All @@ -18,7 +19,7 @@ const mediaQueryList = window.matchMedia('(max-width: 615px), (max-height: 420px
* @slot - Default slot for content inside dialog
* @slot footer - Slot for footer content such as workflow buttons
*/
class DialogFullscreen extends LocalizeCoreElement(AsyncContainerMixin(DialogMixin(LitElement))) {
class DialogFullscreen extends PropertyRequiredMixin(LocalizeCoreElement(AsyncContainerMixin(DialogMixin(LitElement)))) {

static get properties() {
return {
Expand All @@ -32,6 +33,10 @@ class DialogFullscreen extends LocalizeCoreElement(AsyncContainerMixin(DialogMix
* @type {boolean}
*/
noPadding: { type: Boolean, reflect: true, attribute: 'no-padding' },
/**
* REQUIRED: the title for the dialog
*/
titleText: { type: String, attribute: 'title-text', required: true },
/**
* The preferred width (unit-less) for the dialog. Maximum 1170.
*/
Expand Down
8 changes: 7 additions & 1 deletion components/dialog/dialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { dialogStyles } from './dialog-styles.js';
import { getUniqueId } from '../../helpers/uniqueId.js';
import { ifDefined } from 'lit/directives/if-defined.js';
import { LocalizeCoreElement } from '../../helpers/localize-core-element.js';
import { PropertyRequiredMixin } from '../../mixins/property-required/property-required-mixin.js';
import { styleMap } from 'lit/directives/style-map.js';

const mediaQueryList = window.matchMedia('(max-width: 615px), (max-height: 420px) and (max-width: 900px)');
Expand All @@ -19,7 +20,7 @@ const mediaQueryList = window.matchMedia('(max-width: 615px), (max-height: 420px
* @slot - Default slot for content inside dialog
* @slot footer - Slot for footer content such as workflow buttons
*/
class Dialog extends LocalizeCoreElement(AsyncContainerMixin(DialogMixin(LitElement))) {
class Dialog extends PropertyRequiredMixin(LocalizeCoreElement(AsyncContainerMixin(DialogMixin(LitElement)))) {

static get properties() {
return {
Expand All @@ -43,6 +44,11 @@ class Dialog extends LocalizeCoreElement(AsyncContainerMixin(DialogMixin(LitElem
*/
fullHeight: { type: Boolean, attribute: 'full-height' },

/**
* REQUIRED: the title for the dialog
*/
titleText: { type: String, attribute: 'title-text', required: true },

/**
* The preferred width (unit-less) for the dialog
*/
Expand Down
5 changes: 0 additions & 5 deletions components/dialog/test/dialog-fullscreen.axe.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,6 @@ describe('d2l-dialog-fullscreen', () => {
await expect(el).to.be.accessible();
});

it.skip('no title-text', async() => {
const el = await fixture(html`<d2l-dialog-fullscreen opened>My content</d2l-dialog-fullscreen>`);
await expect(el).to.be.accessible();
});

it.skip('tall content', async() => {
const el = await fixture(html`
<d2l-dialog-fullscreen opened title-text="My Dialog">
Expand Down
13 changes: 12 additions & 1 deletion components/dialog/test/dialog-fullscreen.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import '../dialog-fullscreen.js';
import { runConstructor } from '@brightspace-ui/testing';
import { expect, fixture, html, runConstructor } from '@brightspace-ui/testing';
import { createMessage } from '../../../mixins/property-required/property-required-mixin.js';

describe('d2l-dialog-fullscreen', () => {

Expand All @@ -11,4 +12,14 @@ describe('d2l-dialog-fullscreen', () => {

});

describe('properties', () => {

it('throws error when no title-text', async() => {
const el = await fixture(html`<d2l-dialog-fullscreen></d2l-dialog-fullscreen>`);
expect(() => el.flushRequiredPropertyErrors())
.to.throw(TypeError, createMessage(el, 'title-text'));
});

});

});
5 changes: 0 additions & 5 deletions components/dialog/test/dialog.axe.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,6 @@ describe('d2l-dialog', () => {
await expect(el).to.be.accessible();
});

it.skip('no title-text', async() => {
const el = await fixture(html`<d2l-dialog opened>My content</d2l-dialog>`);
await expect(el).to.be.accessible();
});

it.skip('tall content', async() => {
const el = await fixture(html`
<d2l-dialog opened title-text="My Dialog">
Expand Down
11 changes: 11 additions & 0 deletions components/dialog/test/dialog.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import '../dialog.js';
import { expect, fixture, oneEvent, runConstructor } from '@brightspace-ui/testing';
import { createMessage } from '../../../mixins/property-required/property-required-mixin.js';
import { getComposedActiveElement } from '../../../helpers/focus.js';
import { html } from 'lit';

Expand All @@ -13,6 +14,16 @@ describe('d2l-dialog', () => {

});

describe('properties', () => {

it('throws error when no title-text', async() => {
const el = await fixture(html`<d2l-dialog></d2l-dialog>`);
expect(() => el.flushRequiredPropertyErrors())
.to.throw(TypeError, createMessage(el, 'title-text'));
});

});

describe('focus management', () => {

it('should focus on close button if no focusable elements inside', async() => {
Expand Down

0 comments on commit b5adc2b

Please sign in to comment.