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

Cssectomy #327

Closed
wants to merge 66 commits into from
Closed

Cssectomy #327

wants to merge 66 commits into from

Conversation

BnMcGn
Copy link
Contributor

@BnMcGn BnMcGn commented Dec 13, 2022

  • Splits out hard coded CSS values into separate structures to allow complete removal or exchange per issues #183 and #51.
  • Creates a set of utilities to simplify the merging of CSS from various sources.
  • Provides a platform to work toward the remaining goals of #184.

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!

BnMcGn added 30 commits October 3, 2022 10:02
- added local :class, :style, :attr handling to merge-css
Worked in original, but evidently we can get a null here.
- Didn't detect malformed -css-desc
- Sometimes a non :main component should use the toplevel :class and
:style fields as well as the fields supplied in the parts structure.
Adding `:use-toplevel true` to the -css-desc for a particular component
will allow this. :use-toplevel can also be set to `false` for :main.
- Contents of the :attr field should generally be spread in the target
element, not confined to the :attr keyword of the component. There may
be exceptions, but we'll start with this.
- V-table demo working. Add `:flatten-attr false` to re-com component
calls
- Correct parameters to merge-css
- :flatten-attr parameter was awkward, switch to a separate function to
do the job.
- Everything appears to be working
- some work remaining on the buttons
- css for buttons is split out
- Can't reliably pass a closure as a parameter to a hiccup component.
Not sure why not, but best to avoid for now.
- Fixed date class specifier
- Cmerger closure in wrong place
- More accurate. It's not a description
@mike-thompson-day8
Copy link
Contributor

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

@BnMcGn
Copy link
Contributor Author

BnMcGn commented Feb 3, 2023

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

BnMcGn added 11 commits April 1, 2023 09:14
- Scope issue
:right and :bottom weren't being sent through to CSS, causing confusion
about the location of the popup.
- Available in popover-border, popover-content-wrapper
- updates from 2.13.2 -> 2.18.0
- Make things more consistent with upstream master.
- reuse backdrop component from popover.cljs
@kimo-k kimo-k force-pushed the master branch 6 times, most recently from 1c466ca to 60d0594 Compare March 22, 2024 01:39
@kimo-k
Copy link
Contributor

kimo-k commented Mar 25, 2024

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:

  • components accept a :theme prop
  • A single theme can apply to all of re-com's components (or not).
  • components also observe a global theme registry
    • the user can call reg-global-theme to theme their entire app at once.
  • the registry separates themes by purpose:
    • :base - essential functionality, like :position "absolute", callbacks, etc.
    • :main - re-com's main aesthetic, stuff like :color "blue", spacing
    • :user - up to you.
    • :base-variables, main-variables, :user-variables: these add to the shared context, defining global concerns like spacing and color palette.
  • a component can override or extend any registered theme.
  • a theme is a function: props, context -> new-props, new-context.
    • A component should pass some initial context - part-id & state.
    • a vector of themes is a theme, too.
  • for each part, we reduce all the themes to derive its props.
    • this could become an interceptor pattern if we need that
  • We use merge-props & theme/apply in place of merge-css and add-map-to-hiccup-call.
  • All this should make it possible for re-com to integrate with various style frameworks.
  • meta/stylex, shadow-css, and modern plain css have been our driving examples.

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
Copy link
Contributor Author

BnMcGn commented Mar 28, 2024

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
Copy link
Contributor Author

BnMcGn commented Mar 29, 2024

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
Copy link
Contributor

kimo-k commented Mar 29, 2024

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
Copy link
Contributor Author

BnMcGn commented Apr 1, 2024

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
Copy link
Contributor Author

BnMcGn commented Apr 3, 2024

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

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.

3 participants