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

Conversation

twschiller
Copy link
Contributor

Closes #2581

Adds an "initialize" trigger, which fires on DOM attachment vs. appear

@twschiller twschiller requested a review from fregante February 4, 2022 02:11
@twschiller twschiller added this to the 1.5.3 milestone Feb 4, 2022
@@ -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

Comment on lines +448 to +449
(index, element) => {
void this.runTrigger(element as HTMLElement).then((errors) => {
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);

@@ -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?

}
});
},
{ 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
*/
private intervalController: AbortController | null;
private abortController = new AbortController();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a new JS feature by the way. You can initialize properties right here and they'll be called in the constructor. In most cases you can replace:

class {
	  constructor() {
		  this.prop = myInitialization()
	  }
}

with:

class {
	prop = myInitialization()
}

Copy link
Contributor

@fregante fregante Feb 4, 2022

Choose a reason for hiding this comment

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

This actually allows for some coool usage like pre-bound methods:

class {
   constructor() {
     super(id, name, description, icon);
     // Bind so we can pass as callback
     this.boundEventHandler = this.eventHandler.bind(this);
   }
   eventHandler() {
     // Bind me please
   } 
}

becomes:

class {
   eventHandler = () => {
     // Look ma, no constructor()
   }
}

I'm waiting for the lint rule to materialize:sindresorhus/eslint-plugin-unicorn#314

@twschiller
Copy link
Contributor Author

twschiller commented Feb 4, 2022

Definitely looks cleaner - thanks! I changed onCancel to addCancelHandler just to clarify a bit. (Because in React land the onXXX are always called with the actual arguments)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add "initialize" trigger type
2 participants