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

Add root(options, binds) function to API #57

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

sritchie
Copy link
Collaborator

@sritchie sritchie commented Jan 12, 2023

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 a ref. 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 the Camera component (with proxy set to true) and Root takes care of the set() calls.

Here's what the new example I added looks like:

2023-01-12 11 06 15

@@ -166,7 +171,7 @@ export class API {
}
_reset() {
let left;
return (left = this._up != null ? this._up.reset() : undefined) != null
return (left = this._up != null ? this._up._reset() : undefined) != null
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops!

@@ -70,7 +70,7 @@ describe("live properties", () => {

const size1 = p.get("size")
const opacity1 = p.get("opacity")
expect(Math.abs(size1 - 20.5)).toBeLessThan(0.2)
expect(Math.abs(opacity1 - 0.5)).toBeLessThan(0.2)
expect(Math.abs(size1 - 20.5)).toBeLessThan(0.3)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test was flaky on me, so I'm relaxing the bounds again...

// default focus
expect(mathbox.get("focus")).toBe(1);

// set via root and see the change reflected in mathbox.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had another test that exposed #59, so here I'm only testing the non-binds.

@sritchie
Copy link
Collaborator Author

@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...

@ChristopherChudzicki
Copy link
Collaborator

ChristopherChudzicki commented Jan 14, 2023

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:

  1. For this PR, from a mathbox (sans react) perspective, what does the root() method enable library users to do that they couldn't do before?
  2. In mathbox-react, what sort of syntax are you aiming for that isn't currently supported?

Regarding (1), 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?

Regarding (2), 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 😄

@sritchie
Copy link
Collaborator Author

@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:

  1. You're absolutely right that this doesn't allow anything new, since as you say we already have 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.

@sritchie
Copy link
Collaborator Author

  1. I'm not sure what I'm looking at in the story that's related to this PR!

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: ChristopherChudzicki/mathbox-react#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

  • control settings via these hooks
  • have a "ref" option that can pass the renderer, controls instance, etc to the user without them having to dig around the mathbox object at the top level

In 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.)

@ChristopherChudzicki
Copy link
Collaborator

@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.

Screenshot 2023-01-16 at 10 40 30 AM

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.

Mathbox-react

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 and components for mathbox-react. (I don't really think we should add such things to mathbox core.) Going to try and experiment a bit with this today and write more later.

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.

2 participants