Rename some things

[sroussey]

I was confused by reuse of various names (nodes, group(s)) and went and renamed them. This would be a breaking change.

nodes for the editor itself stays the same, but nodes for the editor config becomes nodeTypes. group becomes theme in some places and group becomes inputGroup in others.

[clarkmcc]

This is really the only change I'm concerned about. This restricts the node group config from being used for anything but theming. While that's all it does today, I'm not sure that that will be true forever. Let me noodle on this a bit and see if I can think of any other short-term uses for the node group config that would be restricted by this change.

[sroussey]

Maybe NodeKind or something. Group is used for theme and group is used for input groups inside a node, and soon hopefully, group will mean a group of nodes.

[clarkmcc]

Agreed, the naming will be ambiguous with the advent of node groups. I like nodeKinds and the associated kind reference from the node configs. I think that solves the ambiguity problem without restricting what we can do with the config in the future.

[sroussey]

eh... you end up in the examples with things like kind: 'geometry' should really gets me thinking it does something else. Maybe package is better than kind. Maybe wait until something non theme comes up?

[sroussey]

There are still some things I would like to rename...

One thing is there are valueTypes and inputTypes but they are intermixed.

For example, a valueType could be boolean and there could be two inputTypes say, checkbox and onoffbuttons. Or a valuetype of color, and different color pickers (like the wheel you have now, or maybe just 16 color choices).

[sroussey]

BTW, I tried adding some documentation, but to see what it really looks like, try here:

https://github.com/sroussey/ngraph/blob/rename-some-things/DOCUMENTATION.md

[clarkmcc]

Hm, that's a fascinating idea on valueTypes and inputTypes. I had not considered that idea that multiple input types could be used to produce the same valueType. I don't know that this is just a renaming thing, I think there may be some architectural changes that would need to be made to support an idea like this.

For example, if an input has a color value type, and there are two different color pickers, then we may need to provide the ability to specify an input's input type and value type. Are there other cleaner solutions you can think of?

[sroussey]

inputTypes should be called inputEditors?

[clarkmcc]

Here's an idea, can we make the config inputEditor field just accept a React component? That would probably be simpler. Something like this

{
  "valueTypes": {
    "inputComponent": MyCustomComponent,
    "defaultValue": "Foobar"
  }
}

[sroussey]

I like that the config can be saved as JSON though

[sroussey]

Something I have found confusing has been that you have the id as a property for a bunch of things:

        valueTypes: {
          string: {
            name: 'String',
            color: '#a1a1a1',
            inputEditor: 'value',
            defaultValue: '',
          },
          number: {
            name: 'Number',
            color: '#a1a1a1',
            inputEditor: 'value',
            defaultValue: '0.000',
          },
          boolean: {
            name: 'Boolean',
            color: '#a1a1a1',
            inputEditor: 'checkbox',
            defaultValue: true,
          },
          specularDistribution: {
            name: 'Specular Distribution',
            color: '#06b6d4',
            inputEditor: 'options',
            options: [
              { name: 'GGX', value: 'ggx' },
              { name: 'Beckmann', value: 'beckmann' },
              { name: 'Phong', value: 'phong' },
            ],
            defaultValue: 'GET',
          },

instead of

        valueTypes: [
          {
            id: 'string',
            name: 'String',
            color: '#a1a1a1',
            inputEditor: 'value',
            defaultValue: '',
          },
          {
            id: 'number',
            name: 'Number',
            color: '#a1a1a1',
            inputEditor: 'value',
            defaultValue: '0.000',
          },
          {
            id: 'boolean',
            name: 'Boolean',
            color: '#a1a1a1',
            inputEditor: 'checkbox',
            defaultValue: true,
          },
          {
            id: 'specularDistribution',
            name: 'Specular Distribution',
            color: '#06b6d4',
            inputEditor: 'options',
            options: [
              { name: 'GGX', value: 'ggx' },
              { name: 'Beckmann', value: 'beckmann' },
              { name: 'Phong', value: 'phong' },
            ],
            defaultValue: 'GET',
          }
      ]

[EDIT: actually, I can think of some type system stuff that is easier if setup this way]

[clarkmcc]

I like the idea of an object of inputs instead of an array of inputs where the key is the id. Are you saying that you can think of type reasons why it's better the way that it is? Or is it better for type reasons to change it to an object.

[sroussey]

Better as it is.

[sroussey]

BTW: I did the rename from theme to kind.

[clarkmcc]

Is this ready to go?

[sroussey]

Yes. From a review point of view, this is the only gotcha:

1a7d05f

[clarkmcc]

Hm, how do the HTML types work, if there are no corresponding value types registered in the config?

[sroussey]

I noticed some html errors which is why I changed it. The name of the value type makes its way into the HTML.

This is good:

valueTypes: {
        text: {
          name: "Text",
          defaultValue: "",
          color: "blue",
          inputEditor: "value",
        },

This is wrong

valueTypes: {
        string: {
          name: "Text",
          defaultValue: "",
          color: "blue",
          inputEditor: "value",
        },

Really, I think there should be different nodeEditors: Not "value", make make a real list of them. Like string (max length?), number (can have min and max values), date, time, etc.

[sroussey]

Oh, the second one is wrong because it ends up rendering

<input type="string">

Which is not valid HTML. Input does not support a type value of string.

[sroussey]

Oh, I just noticed that up above there is defaultValue: 'GET', which is not one of the values from the options. Comes from an example somewhere.

[sroussey]

BTW, the commit 1a7d05f I can remove and we can deal with it later. I meant to create a different PR for it anyhow.

[clarkmcc]

I'd like to spend some more time thinking through that change before we merge. I'm happy to merge this without it, or we can hold up this PR until I get a chance to look closer. Up to you

[sroussey]

I pulled it and moved it to another so we can merge this one.

Probably squash commit...