day8 / re-com

A ClojureScript library of reusable components for Reagent
https://re-com.day8.com.au
MIT License
796 stars 147 forks source link

Cssectomy #327

Closed BnMcGn closed 2 months ago

BnMcGn commented 1 year ago

Note to reviewer: Most of the meat of this rather large pull request is in util.cljs. Everything else is a rather repetitious application of the tools added there.

Thanks!

mike-thompson-day8 commented 1 year ago

Just a quick sorry, because we have yet to look at this.
It does seem substantial and, therefor a bit scary.

BnMcGn commented 1 year ago

Yep, beware the big chunk! Thanks for looking at it. No hurry here.

kimo-k commented 3 months ago

Hey, thanks for all the effort you've put into designing this and maintaining the branch.

I've been over the PR a few times, drawing inspiration where I could. We've settled on a design which is not far off. The main differences are in ergonomics and levels of indirection:

I recently merged some of this work into our alpha components - see dropdown for a working example.

Would be great to hear what you think - specifically, whether you can imagine your tailwind project integrating with this design. It could be straightforward to factor each -css-spec into the default base and main themes, and adjust the component render functions to use theme/apply, or themed, a closure similar to your cmerger.

BnMcGn commented 3 months ago

Looks good!

First glance says definitely workable. I can probably eliminate re-com-tailwind as a separate project and just supply it as a re-com theme.

I think I have a few questions, but need to review the code a little more so that I know what I am asking :-)

BnMcGn commented 3 months ago

Things seem to be getting more complex, both in the component code and in the user options. Not surprising, with all the added features.

Here's something that might help: can the themes parameters to the component be simplified down to one :theme (or perhaps :themes) option? I like what you have done with splitting the theme into base, main and variables sections, but does the component need to know about them? There will be less cognitive load on casual users and component authors if the theme merging is done out of their sight. An advanced user who wishes to - say - reuse base and replace main can bundle them together, then pass the bundle in to specific components with the :theme option.

I should be able to look at rewriting the -css-spec stuff in a couple of weeks. I have another project to finish up.

kimo-k commented 3 months ago

Yes, we should do something like that.

When passed :theme my-theme-fn, the current naive behavior will conj your function onto the :user themes. That means at render time, re-com internally applies all the registered themes, and then applies my-theme-fn last. Does this match with your vision, or do you envision something else?

As for advanced usage, I should examine what I already wrote, maybe write some tests. I know it works to pass in :base-theme my-base-theme, which will replace re-com.theme.default/base with my-base-theme in the reduce. It could also make sense to pass in :theme {:base this-fn :main that-fn}, to replace multiple layers.

BnMcGn commented 3 months ago

Yes, probably, it should match what I'm thinking.

Honestly, I'm going to have to make another pass over your code. It hasn't all sunk in yet.

I was mostly reacting to the sight of the main-theme, theme-vars and base-theme keyword parameters of the dropdown render function.

BnMcGn commented 2 months ago

Should close this. Themes looks like the way to go.