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 2 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
27 changes: 6 additions & 21 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 Down Expand Up @@ -231,8 +232,11 @@ class InputColor extends FocusMixin(FormElementMixin(LocalizeCoreElement(LitElem
this._associatedValue = undefined;
this._missingLabelErrorHasBeenThrown = false;
this._opened = false;
this._validatingLabelTimeout = null;
this._value = undefined;
this.addRequiredProperty('label', {
dependentProps: ['type'],
validator: hasValue => this.type !== 'custom' || hasValue
});
}

get associatedValue() { return this._associatedValue; }
Expand All @@ -258,11 +262,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 +276,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 +394,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'));
});

});

});
44 changes: 13 additions & 31 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 @@ -96,20 +97,23 @@ 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
this.addRequiredProperty('label', {
message: defaultMessage => {
if (!this.labelledBy) return defaultMessage;
return `LabelledMixin: "${this.tagName.toLowerCase()}" is labelled-by="${this.labelledBy}", but its label is empty`;
},
validator: hasValue => {
if (!this.labelRequired || hasValue) return true;
if (!this.labelledBy) return false;
return this._labelElem !== null;
}
});
dlockhart marked this conversation as resolved.
Show resolved Hide resolved
}

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 +205,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);
}

};
108 changes: 108 additions & 0 deletions mixins/property-required/property-required-mixin.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
import { dedupeMixin } from '@open-wc/dedupe-mixin';

const TIMEOUT_DURATION = 3000;

export function createDefaultMessage(tagName, propertyName) {
return `${tagName}: "${propertyName}" attribute is required.`;
}

export function createUndefinedPropertyMessage(tagName, propertyName) {
return `PropertyRequiredMixin: "${tagName.toLowerCase()}" has no property "${propertyName}".`;
}

export function createInvalidPropertyTypeMessage(tagName, propertyName) {
return `PropertyRequiredMixin: only String properties can be required ("${tagName}" required property "${propertyName}".`;
}

export const PropertyRequiredMixin = dedupeMixin(superclass => class extends superclass {

constructor() {
super();
this._allProperties = new Map();
this._requiredProperties = new Map();
this._initProperties(Object.getPrototypeOf(this));
}

firstUpdated(changedProperties) {
super.firstUpdated(changedProperties);
for (const name of this._requiredProperties.keys()) {
this._validateRequiredProperty(name);
}
}

updated(changedProperties) {
super.updated(changedProperties);
this._requiredProperties.forEach((value, name) => {
const doValidate = changedProperties.has(name) ||
value.dependentProps.includes(name);
if (doValidate) this._validateRequiredProperty(name);
});
}

addRequiredProperty(name, opts) {

const prop = this._allProperties.get(name);
if (prop === undefined) {
throw new Error(createUndefinedPropertyMessage(this.tagName.toLowerCase(), name));
}

if (prop.type !== String) {
throw new Error(createInvalidPropertyTypeMessage(this.tagName.toLowerCase(), name));
}

opts = {
...{ dependentProps: [], message: defaultMessage => defaultMessage, validator: hasValue => hasValue },
...opts
};

this._requiredProperties.set(name, {
attrName: prop.attribute || name,
dependentProps: opts.dependentProps,
message: opts.message,
thrown: false,
timeout: null,
validator: opts.validator
});

}

flushRequiredPropertyErrors() {
dlockhart marked this conversation as resolved.
Show resolved Hide resolved
for (const name of this._requiredProperties.keys()) {
this._flushRequiredPropertyError(name);
}
}

_flushRequiredPropertyError(name) {

if (!this._requiredProperties.has(name) || !this.isConnected) return;

const info = this._requiredProperties.get(name);
clearTimeout(info.timeout);
info.timeout = null;

const hasValue = this[name]?.constructor === String && this[name]?.length > 0;
const success = info.validator(hasValue);
if (!success) {
if (info.thrown) return;
info.thrown = true;
const defaultMessage = createDefaultMessage(this.tagName.toLowerCase(), info.attrName);
throw new TypeError(info.message(defaultMessage));
}

}

_initProperties(base) {
if (base === null) return;
this._initProperties(Object.getPrototypeOf(base));
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat, I figured there had to be a better/cleaner option than __proto__.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah was going to mention this option to you, as when I was looking up __proto__ the Mozilla docs had all kinds of scary deprecation warnings!

for (const name in base.constructor.properties) {
this._allProperties.set(name, base.constructor.properties[name]);
}
}

_validateRequiredProperty(name) {
const info = this._requiredProperties.get(name);
clearTimeout(info.timeout);
info.timeout = setTimeout(() => this._flushRequiredPropertyError(name), TIMEOUT_DURATION);
}

});
Loading