-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
packages/yew/src/virtual_dom/vtag.rs
Outdated
#[deprecated( | ||
note = "The `set_listener` method is deprecated and will be removed in the future. Use the `set_listeners` method instead." | ||
)] |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
packages/yew/src/virtual_dom/vtag.rs
Outdated
@@ -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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this 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
The average user would not. Having a lower level API is convenient for
specific use cases. If you look into the examples dir, you can see how it
can be used. IIRC the markdown example was one of them.
…On Mon, 27 Dec 2021, 00:33 Julius Lungys, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/yew/src/virtual_dom/vtag.rs
<#2300 (comment)>:
> @@ -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]
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?
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)
—
Reply to this email directly, view it on GitHub
<#2300 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB347MCM7DNPEUAWCPSSQNDUS6J4TANCNFSM5KYXPUDQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you commented.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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
Description
Add new methods
add_pending_listener
andset_listeners
on theVTag
, also deprecate oldset_listener
method.Fixes #2298
Checklist
cargo make pr-flow