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

Remove InputData & ChangeData #2000

Merged
merged 7 commits into from
Aug 16, 2021
Merged

Remove InputData & ChangeData #2000

merged 7 commits into from
Aug 16, 2021

Conversation

mc1098
Copy link
Contributor

@mc1098 mc1098 commented Aug 12, 2021

Description

InputData and ChangeData have been removed as they were overly
restrictive, the implementation would only allow adding listeners to
input, textarea, or select elements and would panic at runtime
otherwise.

TargetCast Trait

pub trait TargetCast
where
    Self: AsRef<Event>,
{
    #[inline]
    fn target_dyn_into<T>(&self) -> Option<T>
    where
        T: AsRef<EventTarget> + JsCast,
    {
        self.as_ref()
            .target()
            .and_then(|target| target.dyn_into().ok())
    }
    
    #[inline]
    fn target_unchecked_into<T>(&self) -> T
    where
        T: AsRef<EventTarget> + JsCast,
    {
        self.as_ref().target().unwrap().unchecked_into()
    }
}

It's one implementation

impl<E: AsRef<Event>> TargetCast for E {}

TargetCast trait should help with migration and getting the value from
inputs etc. It is implemented for every web_sys Event which makes it
useful beyond what InputData and ChangeData provided.
The user is much more likely to know what the target type is
and this saves Yew trying to brute force what element the target type might
be. This also allows users to cast to custom web component types when they
extend EventTarget (which is easily done in wasm_bindgen).

I think this trait is convenient enough to exist on it's own but follows the
same theme of JsCast which might also be a nice way for new users to
be introduced to what JsCast can do at a limited scope.

Updated the examples to use the TargetCast trait on event's where a value is
required from the target of an event.

Fixed instances where the value attribute is applied when this is not
required to do so, this reduces rendering on some examples.

Fixed using string values when floats are required, as the new approach allows for calling
HtmlInputElement::value_as_number.

Examples

TargetCast::target_dyn_into

fn view(&self) -> Html {
    html! {
        <div
            onchange={self.link.batch_callback(|e: Event| {
                if let Some(input) = e.target_dyn_into::<HtmlTextAreaElement>() {
                    Some(Msg::Value(input.value()))   
                } else {
                    None
                }
            })}
        >
            <textarea />
            <input type="text" />
        </div>
    }
}

TargetCast::target_unchecked_into

fn view(&self) -> Html {
    html! {
        <input type="text"
            onchange={self.link.callback(|e: Event| {
                // Safe to use as callback is on an `input` element so this event can
                // only come from this input!
                let input: HtmlInputElement = e.target_unchecked_into();
                Msg::Value(input.value())      
            })}
        />
    }
}

It might be useful to add a util fn for getting and setting the value property but I think this is a more global concern then events even so I'd propose doing this, if wanted, in another PR.

Fixes #1776

Checklist

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

mc1098 added 5 commits August 12, 2021 15:56
InputData and ChangeData have been removed as they were overly
restrictive, the implementation would only allow adding listeners to
input, textarea, or select elements and would panic at runtime
otherwise.

TypedTarget trait should help with migration for getting the value from
inputs etc. The user is much more likely to know what the target type is
and this saves Yew trying to brute force what element the target type might
be.

This is convenient enough trait but follows the same theme of JsCast
which might also be a nice way for new users to be introduced to what
JsCast can do at a limited scope.
Updated to use the TypedTarget trait on event's where a value is
required from the target of an event.

Fixed instances where the `value` attribute is applied when this is not
required to do so, this reduces rendering on some examples.

Fixed using string values when floats are required, as the new approach allows for calling
HtmlInputElement::value_as_number.
@github-actions
Copy link

github-actions bot commented Aug 12, 2021

Visit the preview URL for this PR (updated for commit a3733c7):

https://yew-rs--pr2000-target-cast-v5z5x783.web.app

(expires Sat, 21 Aug 2021 13:06:55 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Event targets are of type `EventTarget` and this should be the minimum
bounds for `TargetCast`.
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.

Looks good.

Can you add an example using this with CustomEvent

examples/boids/Cargo.toml Outdated Show resolved Hide resolved
@mc1098
Copy link
Contributor Author

mc1098 commented Aug 14, 2021

Can you add an example using this with CustomEvent

So add a new example to the examples? for creating and dispatching a Custom Event on the Rust side and then having an event listener catch that event? should the example be casting the event to a Custom Event to read detail?

@ranile
Copy link
Member

ranile commented Aug 14, 2021

Can you add an example using this with CustomEvent

So add a new example to the examples? for creating and dispatching a Custom Event on the Rust side and then having an event listener catch that event? should the example be casting the event to a Custom Event to read detail?

I'm not sure if it should be a new example. Maybe we can add a snippet in API docs?

@mc1098
Copy link
Contributor Author

mc1098 commented Aug 14, 2021

I'm not sure if it should be a new example. Maybe we can add a snippet in API docs?

As in docs.rs or the website?

@ranile
Copy link
Member

ranile commented Aug 14, 2021

I'm not sure if it should be a new example. Maybe we can add a snippet in API docs?

As in docs.rs or the website?

docs.rs
It probably isn't worth it to add it to website, considering it isn't going to be used much

@mc1098
Copy link
Contributor Author

mc1098 commented Aug 14, 2021

Can you add an example using this with CustomEvent

Just realised that Yew doesn't enable the CustomEvent feature in web_sys - is it worth adding this feature just for a snippet?
CustomEvent will work just like Event because it extends this class and it would work for any type imported through wasm_bindgen that extends Event.

@ranile
Copy link
Member

ranile commented Aug 14, 2021

Can you add an example using this with CustomEvent

Just realised that Yew doesn't enable the CustomEvent feature in web_sys - is it worth adding this feature just for a snippet?
CustomEvent will work just like Event because it extends this class and it would work for any type imported through wasm_bindgen that extends Event.

It's fine as-is then. We don't really need to enable the feature just for the snippet, especially since it'll be just like the other events

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.

LGTM

@mc1098 mc1098 mentioned this pull request Aug 15, 2021
13 tasks
@siku2 siku2 merged commit c6099cf into yewstack:master Aug 16, 2021
@mc1098 mc1098 deleted the target-cast branch August 17, 2021 14:58
@mc1098 mc1098 mentioned this pull request Sep 6, 2021
3 tasks
@voidpumpkin voidpumpkin added the A-yew Area: The main yew crate label Nov 26, 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.

onchange listener can't be on any element other than input textare or select
4 participants