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

fix(components): allow A to accept class #295

Closed
wants to merge 2 commits into from

Conversation

caseybaggz
Copy link

closes #1038

Current behavior

A component was compiling the class props pretty gnarly and breaking the UI when combined with Panda CSS via Solid-Start app (non-islands setting).

New behavior

  • Fixes all TS type and eslint errors within component
  • Adds class prop with fixes the issue without conflicting with classList

Other

I tested this pretty heavily in my large Solid-Start app and it seems to work in all scenarios (including using end). Likewise, I'm aware that you typically want to avoid combining class and classList but this seems to work cohesively without causing conflicts.

I would even dare say that the active/inactive class design is probably outdated these days in favor of full control by the user so there is more flexibility of the component overall.

Because solid wants to keep the class and classList segregated, I just can't see how enforcing those internally is fully beneficial to the DX vs. letting the developer decide how they want to manage the styling (whether classes they create or using a lib like Panda-CSS).

@ryansolid
Copy link
Member

Honestly I'd love to get rid of active/inactive. Honestly I'd love to get rid of classList (it was definitely a mistake). The first is at least something I'd entertain if the use cases were covered. I honestly have no idea what the expectation of the class usage and classList is. Someone previously did this big classList rewrite.. it's odd that it doesn't work. In your testing was it the server markup that was broken more so than the client?

@caseybaggz
Copy link
Author

@ryansolid it was actually the reverse! The server version was perfect and the client was all weird like in the issue.

I appreciate that insight too. If there's anything I can do in addition to this to help future solid please let me know. 💪

@ryansolid
Copy link
Member

I'm still trying to figure out what the underlying issue is because the reason it was merged was to prevent the updating class wipes everything else issue. Like updating class will just replace the class and wipe out anything classList did. This is why we never recommend having both on a native element. Ideally for now we'd only have classlist managing both until I can deprecate it.

@caseybaggz
Copy link
Author

Makes sense. It is a weird thing that I can only duplicate in my project (which is pretty large now). I've also already just created this fix locally which fixes it just in case it doesn't get shipped. All the Panda team has figured out is that SR is creating the corrupted component on the client...in my specific app instance (but not on a fresh project using the Basic template for example)? 🤔

@caseybaggz
Copy link
Author

The history of the styling libs has been...

  1. Test using CSS modules - ⛔
  2. Test using Tailwind - works, but clogs up the code and is not scalable for multi-theming ⛔
  3. Test using UnoCSS - works, but buggy and no multi-theming support ⛔
  4. Panda - works, scalable, keeps the code clean ✅
    a. Now seeing FOUC with the A component from SS/Router.

Wondering if one of the prior solutions has some weird corrupted meta that is somehow affecting the build still?

@ryansolid
Copy link
Member

Yeah I can't merge this fix because it risks overwriting. I need a reproduction to try to fix it.

@ryansolid ryansolid closed this Jan 30, 2024
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.

[Bug?]: "A" Component causes FOUC and weird props
2 participants