Pauan / rust-dominator

Zero-cost ultra-high-performance declarative DOM library using FRP signals for Rust!
MIT License
960 stars 62 forks source link

Avoid panic & warn on empty class #79

Open Billy-Sheppard opened 9 months ago

Billy-Sheppard commented 9 months ago

Hi Pauan,

This PR avoids the panic if you pass an empty impl Multistr to DomBuilder::class and warns in the console. Would you be open to merging this? Potentially without the warn (as it does require an extra dependency). Perhaps the message can be improved, very open to discussion/improvement.

Pauan commented 9 months ago

The error is from the browser itself, since empty classes are not allowed in the DOM.

Dominator is just a thin layer on top of the DOM, and we generally try to just match the browser's behavior.

If you want to selectively add a class only in certain situations, you can use apply_if / apply:

.apply_if(true, |dom| dom.class("foo"))
.apply(|dom| {
    if some_complex_condition() {
        dom.class("foo")

    } else {
        dom
    }
})

Or you can use .class_signal to dynamically add / remove the class:

.class_signal("foo", some_signal)
Billy-Sheppard commented 9 months ago

Sure I understand, this PR is more of a convenience layer on top, as it shouldn't add too much bloat (especially if you omit the gloo_console import and message). Fair enough if you just want to match almost perfectly to the existing APIs. I mainly offered this because sometimes it can be hard to track down what and where exactly an issue is, this could provide the element tag name and should also print out the stack, which contains the rust function name.

Pauan commented 9 months ago

It should already print out the line (in your Rust code) which is causing the issue, so if it's not then that's a separate bug that needs to be fixed.

Billy-Sheppard commented 9 months ago

Noted, I shall do some investigation, perhaps there is an issue on my side with console_panic_hook or similar. Thanks.