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.

Sign up for GitHub

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

Jump to bottom

POC - Adding subscription controllers #1901

Closed
wants to merge 4 commits into from
Closed

POC - Adding subscription controllers #1901

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
365512e
Add subscription controllers and hookup to the selection components
svanherk Nov 29, 2021
2307580
Divide the event and for property subscribers into separate controllers
svanherk Nov 30, 2021
015aec7
Add form and validation POC
svanherk Dec 1, 2021
367dda8
Add fix and error control (to be tested)
svanherk Dec 7, 2021
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
44 changes: 20 additions & 24 deletions components/form/form-element-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { isCustomFormElement } from './form-helper.js';
import { LocalizeCoreElement } from '../../lang/localize-core-element.js';
import { ProviderController } from '../../helpers/subscriptionControllers.js';

export class FormElementValidityState {

Expand Down Expand Up @@ -111,10 +112,8 @@ export const FormElementMixin = superclass => class extends LocalizeCoreElement(

constructor() {
super();
this._validationCustomConnected = this._validationCustomConnected.bind(this);
this._onFormElementErrorsChange = this._onFormElementErrorsChange.bind(this);

this._validationCustoms = new Set();
this._validity = new FormElementValidityState({});
/** @ignore */
this.forceInvalid = false;
Expand All @@ -130,7 +129,10 @@ export const FormElementMixin = superclass => class extends LocalizeCoreElement(
this.childErrors = new Map();
this._errors = [];

this.shadowRoot.addEventListener('d2l-validation-custom-connected', this._validationCustomConnected);
this._validationCustomsController = new ProviderController(this, {},
{ eventName: 'd2l-validation-custom-connected' }
);

this.shadowRoot.addEventListener('d2l-form-element-errors-change', this._onFormElementErrorsChange);
}

Expand All @@ -153,6 +155,16 @@ export const FormElementMixin = superclass => class extends LocalizeCoreElement(
return this._validity;
}

connectedCallback() {
super.connectedCallback();
this._validationCustomsController.hostConnected();
}

disconnectedCallback() {
super.disconnectedCallback();
this._validationCustomsController.hostDisconnected();
}

updated(changedProperties) {
if (changedProperties.has('_errors') || changedProperties.has('childErrors')) {
let errors = this._errors;
Expand All @@ -173,11 +185,15 @@ export const FormElementMixin = superclass => class extends LocalizeCoreElement(
}
}

getController() {
return this._validationCustomsController;
}

async requestValidate(showNewErrors = true) {
if (this.noValidate) {
return [];
}
const customs = [...this._validationCustoms].filter(custom => custom.forElement === this || !isCustomFormElement(custom.forElement));
const customs = Array.from(this._validationCustomsController.subscribers.values()).filter(custom => custom.forElement === this || !isCustomFormElement(custom.forElement));
const results = await Promise.all(customs.map(custom => custom.validate()));
const errors = customs.map(custom => custom.failureText).filter((_, i) => !results[i]);
if (!this.validity.valid) {
Expand Down Expand Up @@ -208,14 +224,6 @@ export const FormElementMixin = superclass => class extends LocalizeCoreElement(
return this._errors;
}

validationCustomConnected(custom) {
this._validationCustoms.add(custom);
}

validationCustomDisconnected(custom) {
this._validationCustoms.delete(custom);
}

_onFormElementErrorsChange(e) {
e.stopPropagation();
const errors = e.detail.errors;
Expand All @@ -230,16 +238,4 @@ export const FormElementMixin = superclass => class extends LocalizeCoreElement(
}
}

_validationCustomConnected(e) {
e.stopPropagation();
const custom = e.composedPath()[0];
this.validationCustomConnected(custom);

const onDisconnect = () => {
custom.removeEventListener('d2l-validation-custom-disconnected', onDisconnect);
this.validationCustomDisconnected(custom);
};
custom.addEventListener('d2l-validation-custom-disconnected', onDisconnect);
}

};
27 changes: 12 additions & 15 deletions components/form/form-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { getComposedActiveElement } from '../../helpers/focus.js';
import { getUniqueId } from '../../helpers/uniqueId.js';
import { LocalizeCoreElement } from '../../lang/localize-core-element.js';
import { localizeFormElement } from './form-element-localize-helper.js';
import { ProviderController } from '../../helpers/subscriptionControllers.js';

export const FormMixin = superclass => class extends LocalizeCoreElement(superclass) {

Expand All @@ -27,21 +28,25 @@ export const FormMixin = superclass => class extends LocalizeCoreElement(supercl

this.trackChanges = false;
this._tooltips = new Map();
this._validationCustoms = new Set();
this._errors = new Map();

this._validationCustomsController = new ProviderController(this, {},
{ eventName: 'd2l-validation-custom-connected' }
);

this.addEventListener('d2l-form-errors-change', this._onErrorsChange);
this.addEventListener('d2l-form-element-errors-change', this._onErrorsChange);
this.addEventListener('d2l-validation-custom-connected', this._validationCustomConnected);
}

connectedCallback() {
super.connectedCallback();
this._validationCustomsController.hostConnected();
window.addEventListener('beforeunload', this._onUnload);
}

disconnectedCallback() {
super.disconnectedCallback();
this._validationCustomsController.hostDisconnected();
window.removeEventListener('beforeunload', this._onUnload);
}

Expand All @@ -52,6 +57,10 @@ export const FormMixin = superclass => class extends LocalizeCoreElement(supercl
this.addEventListener('focusout', this._onFormElementChange);
}

getController() {
return this._validationCustomsController;
}

// eslint-disable-next-line no-unused-vars
async requestSubmit(submitter) {
throw new Error('FormMixin.requestSubmit must be overridden');
Expand Down Expand Up @@ -154,7 +163,7 @@ export const FormMixin = superclass => class extends LocalizeCoreElement(supercl
if (isCustomFormElement(ele)) {
return ele.validate(showNewErrors);
} else if (isNativeFormElement(ele)) {
const customs = [...this._validationCustoms].filter(custom => custom.forElement === ele);
const customs = Array.from(this._validationCustomsController.subscribers.values()).filter(custom => custom.forElement === ele);
const results = await Promise.all(customs.map(custom => custom.validate()));
const errors = customs.map(custom => custom.failureText).filter((_, i) => !results[i]);
if (!ele.checkValidity()) {
Expand All @@ -171,16 +180,4 @@ export const FormMixin = superclass => class extends LocalizeCoreElement(supercl
return [];
}

_validationCustomConnected(e) {
e.stopPropagation();
const custom = e.composedPath()[0];
this._validationCustoms.add(custom);

const onDisconnect = () => {
custom.removeEventListener('d2l-validation-custom-disconnected', onDisconnect);
this._validationCustoms.delete(custom);
};
custom.addEventListener('d2l-validation-custom-disconnected', onDisconnect);
}

};
3 changes: 1 addition & 2 deletions components/selection/selection-input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import '../inputs/input-checkbox.js';
import { css, html, LitElement } from 'lit-element/lit-element.js';
import { classMap } from 'lit-html/directives/class-map.js';
import { EventSubscriberController } from '../../helpers/subscriptionControllers.js';
import { ifDefined } from 'lit-html/directives/if-defined.js';
import { LabelledMixin } from '../../mixins/labelled-mixin.js';
import { radioStyles } from '../inputs/input-radio-styles.js';
import { SkeletonMixin } from '../skeleton/skeleton-mixin.js';
import { EventSubscriberController } from '../../helpers/subscriptionControllers.js';

const keyCodes = {
SPACE: 32
};
Expand Down
57 changes: 23 additions & 34 deletions components/validation/validation-custom-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isCustomFormElement } from '../form/form-helper.js';
import { EventSubscriberController, ForPropertySubscriberController } from '../../helpers/subscriptionControllers.js';

export const ValidationCustomMixin = superclass => class extends superclass {

Expand All @@ -11,60 +11,49 @@ export const ValidationCustomMixin = superclass => class extends superclass {

constructor() {
super();
this._forElement = null;

this._eventSubscriberController = new EventSubscriberController(this, {},
{ eventName: 'd2l-validation-custom-connected' }
);
Copy link
Member

Choose a reason for hiding this comment

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

Is the event subscriber needed? Seems like the custom validator isn't expecting descendent elements to fire this event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, the event is used by FormMixin and FormElementMixin to allow for someone to put a d2l-validation-custom element inside of one of them without adding a for id. Like, if d2l-input-text wanted to instead just put a d2l-validation-custom element inside of itself. There are no examples of this on the demo pages and I had to hack something together to see it happen. When we actually implement this, I think it's worth trying to figure out if anyone actually ever uses this, and decide if that's something we'd still want to support.

Copy link
Member

Choose a reason for hiding this comment

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

Whoa! Yeah I'd definitely lean towards just removing that functionality altogether.


this._forPropertySubscriberController = new ForPropertySubscriberController(this,
{ onUnsubscribe: this._onUnsubscribe.bind(this) },
{ forProperty: 'for' }
);
}

get forElement() {
return this._forElement;
if (this.for) {
// Validation custom components only support one for element
return this._forPropertySubscriberController.providers[0];
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest doing an array length check first and return null otherwise. Consumers could have been relying on this being null if no element was connected and now accessing this property will throw an array index out of bounds exception.

} else {
return this._eventSubscriberController.provider;
}
}

connectedCallback() {
super.connectedCallback();
this._updateForElement();
this.dispatchEvent(new CustomEvent('d2l-validation-custom-connected', { bubbles: true }));
this._eventSubscriberController.hostConnected();
}

disconnectedCallback() {
super.disconnectedCallback();
if (isCustomFormElement(this._forElement)) {
this._forElement.validationCustomDisconnected(this);
}
this._forElement = null;
this.dispatchEvent(new CustomEvent('d2l-validation-custom-disconnected'));
this._eventSubscriberController.hostDisconnected();
this._forPropertySubscriberController.hostDisconnected();
}

updated(changedProperties) {
super.updated(changedProperties);

changedProperties.forEach((_, prop) => {
if (prop === 'for') {
this._updateForElement();
}
});
this._forPropertySubscriberController.hostUpdated(changedProperties);
}

async validate() {
throw new Error('ValidationCustomMixin requires validate to be overridden');
}

_updateForElement() {
const oldForElement = this._forElement;
if (this.for) {
const root = this.getRootNode();
this._forElement = root.getElementById(this.for);
if (!this._forElement) {
throw new Error(`validation-custom failed to find element with id ${this.for}`);
}
} else {
this._forElement = null;
}
if (this._forElement !== oldForElement) {
if (isCustomFormElement(oldForElement)) {
oldForElement.validationCustomDisconnected(this);
}
if (isCustomFormElement(this._forElement)) {
this._forElement.validationCustomConnected(this);
}
_onUnsubscribe() {
if (this._forPropertySubscriberController.provider.length === 0) {
throw new Error(`validation-custom failed to find element with id ${this.for}`);
Copy link
Member

Choose a reason for hiding this comment

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

When does this end up getting thrown? Seems like before it would throw during connectedCallback -- which arguably could be too soon if there was a delay rendering the element it's looking for.

Having some kind of built-in error throwing capability in the controller itself might be a nice future enhancement too... could simplify the LabelledBy code as well as it does a lot of its own error throwing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I didn't test this out before/after but wanted to keep it in to remember - I assume it always "just worked" because people are putting the validator right beside the element. I was looking at adding a "waitTime" to handle LabelledBy anyways, I wonder if I could generalize that to always search for three seconds, and then call an error callback. I don't think we'd setup an error callback for selection or filter because it's not the end of the world if the element doesn't exist, they'll just handle themselves accordingly?

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of an error callback a lot. 👍

}
}

Expand Down