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

Add pending event listener on the VTag #2300

Merged
merged 3 commits into from
Dec 28, 2021
Merged

Conversation

XX
Copy link
Contributor

@XX XX commented Dec 26, 2021

Description

Add new methods add_pending_listener and set_listeners on the VTag, also deprecate old set_listener method.

Fixes #2298

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

Comment on lines 447 to 449
#[deprecated(
note = "The `set_listener` method is deprecated and will be removed in the future. Use the `set_listeners` method instead."
)]
Copy link
Member

Choose a reason for hiding this comment

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

Let's just remove this function instead of deprecating this. Breaking changes are fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hamza1311 Ok, I removed this function.

@@ -430,8 +431,28 @@ impl VTag {
.insert(key, value.into_prop_value());
}

/// Set event listeners on the [VTag]'s [Element]
/// Add pending event listener on the [VTag]'s [Element]
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a little note about what a pending listener is here? It isn't clear what pending listener is from just looking at this method.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah what is a "pending event listener"?

Copy link
Contributor

Choose a reason for hiding this comment

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

A pending listener is a Listener instance pending for registration in the global event listener multiplexer. That's an internal implementation detail, that need not be exposed. This function is better named add_listener() and the other public function can be set_listeners(), as you have commited.

set_listener() is a misnomer and not needed. I think it got in here via improper merge conflict resolution on my part.

Copy link
Member

@voidpumpkin voidpumpkin Dec 26, 2021

Choose a reason for hiding this comment

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

I still have not received an answer anywhere why would the average yew user need this?
To my understanding it just skips the html! macro to add a listener?
Cant users that dislike that macro use something like yew-vdom-gen instead of hacking through yew internal api? (I get the irony that yew-vdom-gen uses same internal api)
EDIT: nevermind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bakape @voidpumpkin I renamed this function to add_listener.

@voidpumpkin voidpumpkin added the A-yew Area: The main yew crate label Dec 26, 2021
Copy link
Member

@voidpumpkin voidpumpkin left a comment

Choose a reason for hiding this comment

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

Putting this PR on hold until we resolve the core issue discussion #2298

@bakape
Copy link
Contributor

bakape commented Dec 27, 2021 via email

Copy link
Member

@voidpumpkin voidpumpkin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ranile ranile left a comment

Choose a reason for hiding this comment

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

This would be helpful in yew-vdom-gen. I can replace the internal Vec of listeners with add_listener call

@ranile ranile merged commit f0fb406 into yewstack:master Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return the add_listener method for VTag
4 participants