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

Support for Custom DOM Injection Targets #12

Closed
superstructor opened this issue May 9, 2021 · 6 comments · Fixed by #14
Closed

Support for Custom DOM Injection Targets #12

superstructor opened this issue May 9, 2021 · 6 comments · Fixed by #14
Labels
enhancement New feature or request

Comments

@superstructor
Copy link

Please support injection of <style> tags into custom DOM targets, not just (.-head js/document) which is hard-coded in inject!.

There are at least two use cases for this:

For example, re-frame-10x is rendered into a shadow root then sets a *dom* atom in a slightly modified spade so that <style> tags are rendered inside the shadow dom.

It is not clear to me if the general solution in spade should be also global; i.e. two options would be

  • all defclass / defglobal render to some custom dom set early, or default to (.-head js/document) if not set
  • per defclass / defglobal arg or meta so styles can be rendered to different dom contexts on a case by case basis if needed, or default to (.-head js/document) if not set
@superstructor
Copy link
Author

The external window case is a bit different. Prior to using shadow-dom I had to manually copy @spade.runtime/*injected* to the popup window when it was opened by the user, then add-watch on @spade.runtime/*injected* to add any that get injected after the popup window was already opened.

See https://github.com/day8/re-frame-10x/blob/442c4ad642e3791fe761b15252f0b3563e007f86/src/day8/re_frame_10x/styles.cljs#L521

@dhleong dhleong added the enhancement New feature or request label May 9, 2021
@dhleong
Copy link
Owner

dhleong commented May 9, 2021

Very cool! I didn't know re-frame-10x was using spade, and I definitely wasn't familiar with these use-cases. I think the other use-case that it'd be nice to support is server-side rendering, but I don't have any projects doing that so am even less familiar with exactly what would be required.

Here's some initial ideas I had after glancing at this:

  • Create a "IStyleContainer" protocol with inject-style and update-style methods.
  • Use a *dom* dynamic var, like re-frame-10x is using, which defaults to js/document.head, possibly in an atom that can be replaced globally
  • The default DomStyleContainer will create <style> elements and and update them via *dom*
  • Another implementation of IStyleContainer could be used to write to another atom that could be pulled from for a SSR use-case (or whatever)

The tricky part is providing the *dom* value, at least for React. Restricting a defattrs at the declaration site to a specific target seems like a potential foot-gun, since it breaks composability without any compile-time safety. Even if using eg with-redefs worked for the initial render pass, I think the bindings would have reset for any subsequent reactive updates (not sure/haven't tested). React.Context seems like a good tool here, so the same styles can be used against different targets, but I'm not sure how the style function would be able to pull out the current Context to use, since it's not a component....

@superstructor
Copy link
Author

Spade is a powerful mechanism for integrating Garden styling. We're also going to use it to do the internal styling implementation of re-com in day8/re-com#284 So thanks for Spade @dhleong !

If the *dom* approach is taken, won't that make supporting multiple shadow-dom targets on the same page from the same codebase (e.g. a component library) impossible ?

@dhleong
Copy link
Owner

dhleong commented May 10, 2021

If the *dom* approach is taken, won't that make supporting multiple shadow-dom targets on the same page from the same codebase (e.g. a component library) impossible ?

In fact, I think it may enable this! I haven't tested this at all, but I have a terrible idea for a solution for React/Reagent, based on this. The key is that spade is generating the code for the functions that users use to inject styles. Here's the idea:

  1. Wrap your hiccup tree in [spade-dom <your-target-element> ...]
  2. spade-dom will be a React.Context.Provider
  3. In style functions, pull any preferred dom element out of the spade-dom Context using the above hack, and use (binding [*dom* <value>] ...) to execute the eg: inject! or update!

Non-React(ive) contexts should be able to use (binding) in the same way for one-off rendering. Note that this is a huge hack—ReactN doesn't seem to be terribly active, but it's not apparently abandoned either, so perhaps it's a reasonable hack? You would not be able to change the target DOM element of a rendering, but I think this is somewhat reasonable; it'd be like changing the root element:

(let [shadow-dom ...]
  (react-dom/render [spade-dom shadow-dom [app]] shadow-dom]))

What do you think?

@dhleong
Copy link
Owner

dhleong commented May 16, 2021

See #14 for my first pass PR. Please let me know if this looks like it will suit your needs!

dhleong added a commit that referenced this issue May 29, 2021
* Refactor to support a dynamic "style container" to render CSS into

See #12

* Fix factory fn name extraction in non-jvm context

* Fix incorrect namespace reference

* Fix missing spade.runtime ns reference in JVM context

* Implement React Context-based IStyleContainer-providing solution

* Refactor IStyleContainer and its implementations out of the runtime ns

`runtime` is more of an "implementation detail" ns, so things that
clients might reasonably want to access shouldn't live there.

* Fix old, incorrect ns reference
@dhleong
Copy link
Owner

dhleong commented May 29, 2021

Sorry for the huge delay, things have been busy and I got sidetracked! A snapshot of 1.2.0 just got deployed to clojars (spade-1.2.0-20210529.143734-2) containing the PR. Please try it out and see if it works for you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants