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.

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

https://github.com/clarkmcc/ngraph/pull/23/commits/1a7d05f6a53efb4cb8e7f53c45013d457b2851cf

[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 https://github.com/clarkmcc/ngraph/commit/1a7d05f6a53efb4cb8e7f53c45013d457b2851cf 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...