Open sritchie opened 1 year ago
@ChristopherChudzicki we could also remove rootProps
handling in the Mathbox component if you like and delegate it all to a new Root
component: https://github.com/ChristopherChudzicki/mathbox-react/blob/main/mathbox-react/src/components/Mathbox.tsx#L52...
Hey @sritchie I have looked at this a bit and will try to look more closely today. Thanks for drawing my attention to the <camera />
component...I had not used it and it seems very useful.
Two questions for you:
mathbox
(sans react) perspective, what does the root()
method enable library users to do that they couldn't do before?mathbox-react
, what sort of syntax are you aiming for that isn't currently supported?mathbox
It seems to me that the functionality added is "enable selecting root node from any node". I.e, whereas calling selection.line()
, selection.point()
, etc creates a new node of that type with selection
as the parent, calling selection.root()
does not create a new node, it returns a reference to the root element:
const mathbox = new MathBox({ ... }) // then add nodes
const selection = mathbox.select('whatever_css_selector')
selection.root() === mathbox // true, same object
// this
selection.root(props, liveProps)
// exactly equivalent to
mathbox.set(props)
mathbox.bind(liveProps)
The new part is "now selection
can get a reference to the root node, even if no such reference was in scope before".
Is my understanding correct? What patterns does this enable beyond the existing API?
mathbox-react
Regarding the desired mathbox-react experience... Is this roughly what you're interested in? https://github.com/ChristopherChudzicki/mathbox-react/pull/238/files
Incidentally... cameras with proxy=true
AND a live position don't work great 😄
@ChristopherChudzicki okay, here we go!
First quick note: I loaded up your story with live position and proxy=true and it looks pretty good to my eye. What do you think is wrong there?
Now your Qs:
set
and bind
. The only thing this added for me was help with discovering the API. It took me a while (it still is!) to figure out what methods are available and what options are supported for each primitive, since everything is sewn together dynamically.In retrospect, these docs DO cover it all: https://github.com/unconed/mathbox/blob/master/docs/primitives.md#base/root
but figuring out how each primitive uses each option is tough. So it felt appropriate to add a method for the one missing primitive.
BUT I'm fine abandoning that and adding more docs if this feels like extra clutter.
Separate comment coming for 2.
I am becoming more attracted to the idea of components that exist only to install a bunch of hooks that can reactively update config settings. I've been thinking through your observations here: https://github.com/ChristopherChudzicki/mathbox-react/issues/24
In mathbox.cljs, I have a version of the Mathbox
component that can take additional options for "renderer"
and "controls"
. This allows me to reactively control background-color
, background-opacity
(for the renderer) and maxDistance
and rotateSpeed
(for controls)` using effects that update those settings.
(The code lives here, take a look at the hooks: https://github.com/mentat-collective/mathbox.cljs/pull/9/files#diff-2005ffcffcad9436899682298be933ef32374e5c6a60e864aa74b120b47b32baR33-R82)
It feels like it would be a BETTER pattern instead to make separate Renderer
and Controls
components that don't add anything to the tree, but
mathbox
object at the top levelIn fact I think this might be the answer to configuring all of the threestrap plugins. Camera with proxy true is sort of this kind of component already, would you agree?
If we embrace this design, I think having a Root
component makes sense. We could then remove the rootProps
from the Mathbox
constructor, and have the user configure them (both static and liveProps) with this component.
@ChristopherChudzicki let me know what you think of this. (Of course we can add a Root
component without adding a root(options, binds)
to the API, so this discussion has diverged from the original PR, as you anticipated.)
@sritchie re
The only thing this added for me was help with discovering the API... So it felt appropriate to add a method for the one missing primitive
Regarding discoverability, do you think it would help to change all the examples from const mathbox = new Mathbox(...)
to const root = new Mathbox()
? The name mathbox
makes sense, but the name root
more explicitly tells people that the MAthbox constructor returns the root node.
My approach for discoverability has been to add the TS types, though I do not know how much clojure benefits from those, if at all.
The two things that give me reason I hesitate to add this is that the root()
method is that it is different from all the others: whereas other methods create a new node, this one retrieves an existing node (the root node). But that's probably OK.
On the other hand, I do see clear benefit in adding a <Root />
component to mathbox react: Then <Mathbox >
is analogous to new Mathbox()
and <Root />
is analogous to the return value of new Mathbox()
.
I like the idea about a
This PR adds the
root()
function that I had been missing! The advantage here in a mathbox-react context is that you can set root properties now without messing with aref
. This simplifies my initialization code...@ChristopherChudzicki , after this is in I think we can add a
Root
component to mathbox-react, yeah? Then all of the initial camera settings from the examples can be replaced with uses of theCamera
component (with proxy set to true) andRoot
takes care of theset()
calls.Here's what the new example I added looks like: