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

PropertyRequiredMixin #4272

Merged
merged 9 commits into from
Nov 23, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
31 changes: 9 additions & 22 deletions components/inputs/input-color.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { getValidHexColor } from '../../helpers/color.js';
import { ifDefined } from 'lit/directives/if-defined.js';
import { inputLabelStyles } from './input-label-styles.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 DEFAULT_VALUE = '#000000';
Expand Down Expand Up @@ -80,7 +81,7 @@ const SWATCH_TRANSPARENT = `<svg xmlns="http://www.w3.org/2000/svg" width="24" h
* This component allows for inputting a HEX color value.
* @fires change - Dispatched when an alteration to the value is committed by the user.
*/
class InputColor extends FocusMixin(FormElementMixin(LocalizeCoreElement(LitElement))) {
class InputColor extends PropertyRequiredMixin(FocusMixin(FormElementMixin(LocalizeCoreElement(LitElement)))) {

static get properties() {
return {
Expand All @@ -103,7 +104,13 @@ class InputColor extends FocusMixin(FormElementMixin(LocalizeCoreElement(LitElem
* REQUIRED: Label for the input, comes with a default value for background & foreground types.
* @type {string}
*/
label: { type: String },
label: {
type: String,
required: {
dependentProps: ['type'],
validator: (_value, elem, hasValue) => elem.type !== 'custom' || hasValue
}
},
/**
* Hides the label visually
* @type {boolean}
Expand Down Expand Up @@ -231,7 +238,6 @@ class InputColor extends FocusMixin(FormElementMixin(LocalizeCoreElement(LitElem
this._associatedValue = undefined;
this._missingLabelErrorHasBeenThrown = false;
this._opened = false;
this._validatingLabelTimeout = null;
this._value = undefined;
}

Expand All @@ -258,11 +264,6 @@ class InputColor extends FocusMixin(FormElementMixin(LocalizeCoreElement(LitElem
return '#opener';
}

firstUpdated(changedProperties) {
super.firstUpdated(changedProperties);
this._validateLabel();
}

render() {

const label = !this.labelHidden ? html`<div class="d2l-input-label">${this._getLabel()}</div>` : nothing;
Expand All @@ -277,8 +278,6 @@ class InputColor extends FocusMixin(FormElementMixin(LocalizeCoreElement(LitElem

super.updated(changedProperties);

if (changedProperties.has('label') || changedProperties.has('type')) this._validateLabel();
Copy link
Member Author

Choose a reason for hiding this comment

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

This use case forced me to add support for "dependent properties" -- essentially other properties to watch and re-validate when they change.


if (changedProperties.has('value') || changedProperties.has('type') || changedProperties.has('disallowNone')) {
this.setFormValue(this.value);
}
Expand Down Expand Up @@ -397,17 +396,5 @@ class InputColor extends FocusMixin(FormElementMixin(LocalizeCoreElement(LitElem
));
}

_validateLabel() {
clearTimeout(this._validatingLabelTimeout);
// don't error immediately in case it doesn't get set immediately
this._validatingLabelTimeout = setTimeout(() => {
this._validatingLabelTimeout = null;
const hasLabel = (typeof this.label === 'string') && this.label.length > 0;
if (!hasLabel && this.type === 'custom') {
throw new Error('<d2l-input-color>: "label" attribute is required when "type" is "custom"');
}
}, 3000);
}

}
customElements.define('d2l-input-color', InputColor);
32 changes: 32 additions & 0 deletions components/inputs/test/input-color.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import '../input-color.js';
import { expect, fixture, html, oneEvent, runConstructor } from '@brightspace-ui/testing';
import { createDefaultMessage } from '../../../mixins/property-required/property-required-mixin.js';

describe('d2l-input-color', () => {

Expand Down Expand Up @@ -128,4 +129,35 @@ describe('d2l-input-color', () => {

});

describe('validation', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Example of how a component can now test that the validation is set up properly.


['foreground', 'background'].forEach(type => {
it(`should not require a label when type is "${type}"`, async() => {
const elem = await fixture(html`<d2l-input-color type="${type}"></d2l-input-color>`);
expect(() => elem.flushRequiredPropertyErrors()).to.not.throw();
});
});

it('should throw when type is "custom" and no label', async() => {
const elem = await fixture(html`<d2l-input-color type="custom"></d2l-input-color>`);
expect(() => elem.flushRequiredPropertyErrors())
.to.throw(TypeError, createDefaultMessage('d2l-input-color', 'label'));
});

it('should not throw when type is "custom" and label is provided', async() => {
const elem = await fixture(html`<d2l-input-color label="value" type="custom"></d2l-input-color>`);
expect(() => elem.flushRequiredPropertyErrors()).to.not.throw();
});

it('should require a label when type changes to "custom"', async() => {
const elem = await fixture(html`<d2l-input-color type="foreground"></d2l-input-color>`);
expect(() => elem.flushRequiredPropertyErrors()).to.not.throw();
elem.setAttribute('type', 'custom');
await elem.updateComplete;
expect(() => elem.flushRequiredPropertyErrors())
.to.throw(TypeError, createDefaultMessage('d2l-input-color', 'label'));
});

});

});
29 changes: 12 additions & 17 deletions components/tag-list/tag-list-item-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { classMap } from 'lit/directives/class-map.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 { RtlMixin } from '../../mixins/rtl/rtl-mixin.js';

const keyCodes = {
Expand All @@ -18,7 +19,7 @@ const keyCodes = {
SPACE: 32
};

export const TagListItemMixin = superclass => class extends LocalizeCoreElement(RtlMixin(superclass)) {
export const TagListItemMixin = superclass => class extends LocalizeCoreElement(PropertyRequiredMixin(RtlMixin(superclass))) {

static get properties() {
return {
Expand All @@ -39,7 +40,16 @@ export const TagListItemMixin = superclass => class extends LocalizeCoreElement(
/**
* @ignore
*/
keyboardTooltipShown: { type: Boolean, attribute: 'keyboard-tooltip-shown' }
keyboardTooltipShown: { type: Boolean, attribute: 'keyboard-tooltip-shown' },
/**
* @ignore
*/
_plainText: {
state: true,
required: {
message: (_value, elem) => `TagListItemMixin: "${elem.tagName.toLowerCase()}" called "_renderTag()" with empty "plainText" option`
}
}
};
}

Expand Down Expand Up @@ -136,7 +146,6 @@ export const TagListItemMixin = superclass => class extends LocalizeCoreElement(
this.keyboardTooltipShown = false;
this._id = getUniqueId();
this._plainText = '';
this._validatingPlainTextTimeout = null;
}

firstUpdated(changedProperties) {
Expand Down Expand Up @@ -217,7 +226,6 @@ export const TagListItemMixin = superclass => class extends LocalizeCoreElement(
}

_renderTag(tagContent, options = {}) {
this._validatePlainText();
this._plainText = options.plainText || '';

const buttonText = this.localize('components.tag-list.clear', { value: this._plainText });
Expand Down Expand Up @@ -277,17 +285,4 @@ export const TagListItemMixin = superclass => class extends LocalizeCoreElement(
`;
}

_validatePlainText() {
clearTimeout(this._validatingPlainTextTimeout);
// don't error immediately in case it doesn't get set immediately
this._validatingPlainTextTimeout = setTimeout(() => {
this._validatingPlainTextTimeout = null;
if (!this.isConnected) return;
const hasPlainText = (this._plainText?.constructor === String) && this._plainText?.length > 0;
if (!hasPlainText) {
throw new Error(`TagListItemMixin: "${this.tagName.toLowerCase()}" called "_render()" with empty "plainText" option`);
}
}, 3000);
}

};
48 changes: 16 additions & 32 deletions mixins/labelled/labelled-mixin.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@

import { cssEscape } from '../../helpers/dom.js';
import { PropertyRequiredMixin } from '../property-required/property-required-mixin.js';

const getCommonAncestor = (elem1, elem2) => {

Expand Down Expand Up @@ -74,7 +75,7 @@ export const LabelMixin = superclass => class extends superclass {

};

export const LabelledMixin = superclass => class extends superclass {
export const LabelledMixin = superclass => class extends PropertyRequiredMixin(superclass) {

static get properties() {
return {
Expand All @@ -87,7 +88,20 @@ export const LabelledMixin = superclass => class extends superclass {
* REQUIRED: Explicitly defined label for the element
* @type {string}
*/
label: { type: String }
label: {
type: String,
required: {
message: (_value, elem, defaultMessage) => {
if (!elem.labelledBy) return defaultMessage;
return `LabelledMixin: "${elem.tagName.toLowerCase()}" is labelled-by="${elem.labelledBy}", but its label is empty`;
},
validator: (_value, elem, hasValue) => {
if (!elem.labelRequired || hasValue) return true;
if (!elem.labelledBy) return false;
return elem._labelElem !== null;
}
}
}
};
}

Expand All @@ -96,20 +110,12 @@ export const LabelledMixin = superclass => class extends superclass {
this.labelRequired = true;
this._labelElem = null;
this._missingLabelErrorHasBeenThrown = false;
this._validatingLabelTimeout = null;
}

firstUpdated(changedProperties) {
super.firstUpdated(changedProperties);
this._validateLabel(); // need to check this even if "label" isn't updated in case it's never set
}

async updated(changedProperties) {

super.updated(changedProperties);

if (changedProperties.has('label')) this._validateLabel();

if (!changedProperties.has('labelledBy')) return;

if (!this.labelledBy) {
Expand Down Expand Up @@ -201,26 +207,4 @@ export const LabelledMixin = superclass => class extends superclass {

}

_validateLabel() {
clearTimeout(this._validatingLabelTimeout);
// don't error immediately in case it doesn't get set immediately
this._validatingLabelTimeout = setTimeout(() => {
this._validatingLabelTimeout = null;
const hasLabel = (typeof this.label === 'string') && this.label.length > 0;
if (this.isConnected && !hasLabel) {
if (this.labelledBy) {
if (this._labelElem) {
this._throwError(
new Error(`LabelledMixin: "${this.tagName.toLowerCase()}" is labelled-by="${this.labelledBy}", but its label is empty`)
);
}
} else {
this._throwError(
new Error(`LabelledMixin: "${this.tagName.toLowerCase()}" is missing a required "label" attribute`)
);
}
}
}, 3000);
}

};
105 changes: 105 additions & 0 deletions mixins/property-required/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
# PropertyRequiredMixin

This mixin will make a component's property "required". If a value is not provided for a required property, an exception will be thrown asynchronously.

Required properties are useful in situations where a missing value would put the component into an invalid state, such as an accessibility violation.

Only `String` properties can be required.

## Making a Property Required

To require a property, simply set `required` to `true` in the reactive Lit property definition:

```javascript
import { PropertyRequiredMixin } from '@brightspace-ui/core/mixins/property-required/property-required-mixin.js';

class MyElem extends PropertyRequiredMixin(LitElement) {
static properties = {
prop: { type: String, required: true }
};
}
```

If a non-empty `String` value for `prop` is not provided, an exception will be thrown after a few seconds.

## Custom Validation

By default, validating a required property involves ensuring that it's a non-empty `String`. To customize this logic, set `required` to an object and provide a `validator` delegate.

```javascript
static properties = {
mustBeFoo: {
required: {
validator: (value, elem, hasValue) => {
return value === 'foo';
}
},
type: String
}
};
```

The `validator` will be passed the current property `value`, a reference to the element and the result of the default validation.

## Custom Messages

To customize the exception message that gets thrown when validation fails, set `required` to an object and provide a `message` delegate.

```javascript
static properties = {
prop: {
required: {
message: (value, elem, defaultMessage) => {
return `"prop" on "${elem.tagName}" is required.`;
}
},
type: String
}
};
```

The `message` delegate will be passed the current property `value`, a reference to the element and the default message.

## Dependent Properties

The mixin will automatically listen for updates to a required property and re-run the `validator` when the value changes. If custom validation logic relies on multiple properties, list them in `dependentProps`.

```javascript
static properties = {
prop: {
required: {
dependentProps: ['isReallyRequired']
validator: (value, elem, hasValue) => {
return elem.isReallyRequired && hasValue;
}
},
type: String
},
isReallyRequired: { type: Boolean }
};
```

## Unit Testing

If no custom `validator` is used, tests that assert the correct required property errors are thrown are unnecessary -- `@brightspace-ui/core` already covers those tests.

Required property exceptions are thrown asynchronously multiple seconds after the component has rendered. For unit tests, this makes catching them challenging.

To help, use the `flushRequiredPropertyErrors()` method.

```javascript
const tag = defineCE(
class extends PropertyRequiredMixin(LitElement) {
static properties = {
prop: {
type: String,
required: {
validator: (value) => value === 'valid'
}
}
};
}
);
const elem = await fixture(`<${tag} prop="invalid"></${tag}>`);
expect(() => elem.flushRequiredPropertyErrors()).to.throw();
```
Loading