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
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 22 additions & 1 deletion packages/yew/src/virtual_dom/vtag.rs
Expand Up @@ -8,6 +8,7 @@ use std::borrow::Cow;
use std::cmp::PartialEq;
use std::hint::unreachable_unchecked;
use std::marker::PhantomData;
use std::mem;
use std::ops::Deref;
use std::rc::Rc;
use wasm_bindgen::JsCast;
Expand Down Expand Up @@ -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

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.

pub fn add_pending_listener(&mut self, listener: Rc<dyn Listener>) -> bool {
if let Listeners::Pending(listeners) = &mut self.listeners {
let mut listeners = mem::take(listeners).into_vec();
listeners.push(Some(listener));

self.set_listeners(listeners.into_boxed_slice());
true
} else {
false
voidpumpkin marked this conversation as resolved.
Show resolved Hide resolved
}
}

#[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.

pub fn set_listener(&mut self, listeners: Box<[Option<Rc<dyn Listener>>]>) {
self.set_listeners(listeners)
}

/// Set event listeners on the [VTag]'s [Element]
pub fn set_listeners(&mut self, listeners: Box<[Option<Rc<dyn Listener>>]>) {
self.listeners = Listeners::Pending(listeners);
}

Expand Down