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

When using a "computed key" in classList, a following class declaration overwrites the classes set by classList #2050

Open
danieltroger opened this issue Jan 26, 2024 · 4 comments

Comments

@danieltroger
Copy link

Describe the bug

classList doesn't work when used together with class, even when classList is not reactive, if the object looks like this { ["test"]: true } instead of { test: true }

Your Example Website or App

https://playground.solidjs.com/anonymous/a669dae1-46b2-4366-9df0-91d6527908c6

Steps to Reproduce the Bug or Issue

  1. Go to https://playground.solidjs.com/anonymous/a669dae1-46b2-4366-9df0-91d6527908c6
  2. See class-2 being logged to the console

Expected behavior

  1. class-1 being logged to the console

Screenshots or Videos

No response

Platform

  • OS: macOS 14
  • Browser: Latest FirefoxDeveloperEdition

Additional context

No response

@danieltroger danieltroger changed the title When using a "computed key" in classList, a following class declaration overwrites the classList set class When using a "computed key" in classList, a following class declaration overwrites the classes set by classList Jan 26, 2024
@ryansolid
Copy link
Member

Technically as far is it concerned class is reactive here because you are using props.class syntax. It can't know that it isn't. Reactive class means there is always potential for it to overwrite. We can't do inline optimizations on classList with computed keys currently. I suppose the compiler could be more optimized. But this is generally the problem with reactive class + classList and why there is intention to do something about classList in the future.

@danieltroger
Copy link
Author

danieltroger commented Jan 27, 2024

Hmm, I thought the rules was that one can use classList and class together as long as only one is reactive.

We can't do inline optimizations on classList with computed keys currently.

But I didn't know about this :( Why is it not possible? I need computed keys to get dashes in the class lol.

If I was solid I'd tackle the problem by splitting class by (space) and then feeding the output into classList, what's wrong with that approach? Is it really so important to accurately reflect double spaces and other imperfections in the class attribute value? (Like class1 class2 instead of class1 class2)

Edit:

I need computed keys to get dashes in the class lol.

I'm obviously stupid, I can just do { "stuff-with-dashes": true }

@ryansolid
Copy link
Member

To be fair computed property compilation is probably possible if the computed property itself isn't dynamic (a signal or member expression). Also a literal could be eliminated too. These are all optimization edge cases. I just bailed out the optimization sooner because of sake of time and some complexity I hit.

That being said this behavior due to the way class + classList work in general is sort of by design. Poor design, perhaps but it is the way it works right now. I can't compile this problem away in the general sense and it is something that is known issue. Nothing to fix right now other than hope to change the design in the future.

@danieltroger
Copy link
Author

Nothing to fix right now other than hope to change the design in the future.

That's totally fair. Thanks for the information! But I still wonder about your reasoning regarding why not split class into classList in the JSX transformation output for now to fix all the bugs, at the expense of some performance?

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

No branches or pull requests

2 participants