Skip to content

Conversation

@Billy-Sheppard
Copy link
Contributor

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
Copy link
Owner

Pauan commented Oct 10, 2023

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
Copy link
Contributor Author

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
Copy link
Owner

Pauan commented Oct 10, 2023

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants