search

solidjs / solid-router

A universal router for Solid inspired by Ember and React Router
MIT License
1.14k stars 147 forks source link

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

Closed caseybaggz closed 8 months ago

caseybaggz commented 1 year ago

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

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 8 months ago

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