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

#2581: add initialize trigger #2584

Merged
merged 3 commits into from
Feb 4, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 5 additions & 0 deletions src/devTools/editor/tabs/trigger/TriggerConfiguration.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ function supportsSelector(trigger: Trigger) {
}

function supportsTargetMode(trigger: Trigger) {
// XXX: why doesn't `appear` support target mode?
return supportsSelector(trigger) && trigger !== "appear";
}

Expand All @@ -50,6 +51,9 @@ const TriggerConfiguration: React.FC<{
if (!supportsSelector(nextTrigger)) {
setFieldValue("extensionPoint.definition.rootSelector", null);
setFieldValue("extensionPoint.definition.attachMode", null);
}

if (!supportsTargetMode(nextTrigger)) {
setFieldValue("extensionPoint.definition.targetMode", null);
}

Expand All @@ -75,6 +79,7 @@ const TriggerConfiguration: React.FC<{
>
<option value="load">Page Load</option>
<option value="interval">Interval</option>
<option value="initialize">Initialize</option>
<option value="appear">Appear</option>
<option value="click">Click</option>
<option value="dblclick">Double Click</option>
Expand Down
2 changes: 1 addition & 1 deletion src/devTools/editor/toolbar/ReloadToolbar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ function isPanelElement(element: FormState | null): boolean {
* @param element
*/
function isAutomaticTrigger(element: FormState): boolean {
const automatic = ["load", "appear", "interval"];
const automatic = ["load", "appear", "initialize", "interval"];
return (
element?.type === "trigger" &&
automatic.includes(element?.extensionPoint.definition.trigger)
Expand Down
55 changes: 53 additions & 2 deletions src/extensionPoints/triggerExtension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,10 @@ export type Trigger =
| "interval"
// `appear` is triggered when an element enters the user's viewport
| "appear"
| "click"
// `initialize` is triggered when an element is added to the DOM
| "initialize"
Copy link
Contributor

Choose a reason for hiding this comment

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

Any other names? Maybe "add", "insert", "create"?

Copy link
Contributor Author

@twschiller twschiller Feb 4, 2022

Choose a reason for hiding this comment

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

In the configuration I'm going to keep as-is with initialize. That's how JS libraries seem to refer to it

We'll have to change the names of events in the page editor. E.g., non-developers don't know what a "blur" handler is

| "blur"
| "click"
| "dblclick"
| "mouseover"
| "change";
Expand Down Expand Up @@ -155,6 +157,12 @@ export abstract class TriggerExtensionPoint extends ExtensionPoint<TriggerConfig
*/
private cancelWatchNewElements: (() => void) | null;

/**
* Cancel "initialize" trigger observer
* @private
*/
private initializeObserver: MutationObserver | undefined;

/**
* Observer to watch for new elements to appear, or undefined if the trigger is not an `appear` trigger
* @private
Expand Down Expand Up @@ -214,6 +222,8 @@ export abstract class TriggerExtensionPoint extends ExtensionPoint<TriggerConfig
// Clean up observers
this.cancelInitialWaitElements?.();
this.cancelWatchNewElements?.();
this.initializeObserver?.disconnect();
this.initializeObserver = null;
this.appearObserver?.disconnect();
this.appearObserver = null;
this.cancelInitialWaitElements = null;
Expand Down Expand Up @@ -418,6 +428,37 @@ export abstract class TriggerExtensionPoint extends ExtensionPoint<TriggerConfig
}
}

private attachInitializeTrigger(
$element: JQuery<Document | HTMLElement>
): void {
this.initializeObserver?.disconnect();
this.initializeObserver = null;

// The caller will have already waited for the element. So $element will contain at least one element
if (this.attachMode === "once") {
for (const element of $element.get()) {
void this.runTrigger(element);
}

return;
}

this.initializeObserver = initialize(
this.triggerSelector,
(index, element) => {
void this.runTrigger(element as HTMLElement).then((errors) => {
Comment on lines +411 to +412
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We can use a regular async function here

Suggested change
(index, element) => {
void this.runTrigger(element as HTMLElement).then((errors) => {
async (index, element) => {
const errors = await this.runTrigger(element as HTMLElement);

if (errors.length > 0) {
console.error("An error occurred while running a trigger", {
errors,
});
notifyError("An error occurred while running a trigger");
}
});
},
{ target: document }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the default

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently not… :(

);
}

private attachAppearTrigger($element: JQuery): void {
// https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API

Expand Down Expand Up @@ -525,9 +566,14 @@ export abstract class TriggerExtensionPoint extends ExtensionPoint<TriggerConfig
}

private cancelObservers() {
this.intervalController?.abort();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We could use abort controllers for everything so we don't have to have one property for each. For example there can be just one controller register in the constructor and then all the observers attach an abort listener to it. Then this function can just be:

private cancelObservers() {
    this.abortController.abort();
}

Copy link
Contributor Author

@twschiller twschiller Feb 4, 2022

Choose a reason for hiding this comment

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

Good idea, the current state is getting super messy. Will see what will be involved in a cleanup and decide if I want to do it in this PR or a separate one

If you have some spare cycles, could you commit to this PR? I'll wait to merge

Copy link
Contributor

Choose a reason for hiding this comment

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

On it

Copy link
Contributor

Choose a reason for hiding this comment

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

Pushed. This is what it looks like, what do you think?

this.appearObserver?.disconnect();
this.appearObserver = null;
this.initializeObserver?.disconnect();
this.initializeObserver = null;
this.cancelInitialWaitElements?.();
this.cancelWatchNewElements?.();
this.cancelInitialWaitElements = null;
this.cancelWatchNewElements?.();
this.cancelWatchNewElements = null;
}

Expand All @@ -550,6 +596,11 @@ export abstract class TriggerExtensionPoint extends ExtensionPoint<TriggerConfig
break;
}

case "initialize": {
this.attachInitializeTrigger($root);
break;
}

case "appear": {
this.assertElement($root);
this.attachAppearTrigger($root);
Expand Down