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

change from callbacks to events #33

Merged
merged 5 commits into from
Nov 8, 2022
Merged

change from callbacks to events #33

merged 5 commits into from
Nov 8, 2022

Conversation

QingAn
Copy link
Contributor

@QingAn QingAn commented Aug 23, 2022

Related to #30


Preview | Diff

@QingAn
Copy link
Contributor Author

QingAn commented Aug 29, 2022

@Sharonzd, @MichaelWangzitao

As discussed during last week WG call. Please take a look.

@MichaelWangzitao
Copy link

I reviewed this PR, and basically, I agree to merge it to address #30 . In addition, I suggest adding the interaction process between the event handler and attributes. For example, when the event "ongloballaunched" is triggered, "globalState" should be set to "launched".

@Sharonzd
Copy link

Sharonzd commented Oct 20, 2022

I totally agree to change callbacks to events.
From another perspective, this would make related code together, which This conforms to some code design principles like high cohesion.
But we maybe need some examples of how to listen handlers?
Like Page.registerLifeCycle( 'hide', hideEventHandler)

@QingAn
Copy link
Contributor Author

QingAn commented Oct 20, 2022

@Sharonzd

I totally agree to change callbacks to events.

Thanks for reply.

From another perspective, this would make related code together, which This conforms to some code design principles like high cohesion.
But we maybe need some examples of how to listen handlers?
Like Page.registerLifeCycle( 'hide', hideEventHandler)

I agree. By looking at other related spec like Page Lifecycle, I am thinking of the example like:

const doGlobalLaunched = (inputObject) => {
console.log(inputObject.pagePath);
};

global.addEventListener('global launched', doGlobalLaunched);

@QingAn
Copy link
Contributor Author

QingAn commented Oct 20, 2022

@MichaelWangzitao

I reviewed this PR, and basically, I agree to merge it to address #30 .

Thanks for reply.

In addition, I suggest adding the interaction process between the event handler and attributes. For example, when the event "ongloballaunched" is triggered, "globalState" should be set to "launched".

How about the wording like this:
The ongloballaunched attribute is an event handler IDL attribute for the initialization event type. Once ongloballaunched is triggered, globalState will be set to launched.

@MichaelWangzitao
Copy link

@MichaelWangzitao

I reviewed this PR, and basically, I agree to merge it to address #30 .

Thanks for reply.

In addition, I suggest adding the interaction process between the event handler and attributes. For example, when the event "ongloballaunched" is triggered, "globalState" should be set to "launched".

How about the wording like this: The ongloballaunched attribute is an event handler IDL attribute for the initialization event type. Once ongloballaunched is triggered, globalState will be set to launched.

Agree!A brief description is good.

@QingAn QingAn merged commit 1792f3d into main Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants