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

Avoid panic & warn on empty class #79

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

None yet

2 participants