added an ability to set custom css classes for Node's field and value

[maestr0]

No description provided.

[maestr0]

• ability to set custom css classes for Node's field and value
• update for gulp 4.0.0

editor.node.setFieldCssClass('myCustomCssClass');
editor.node.setValueCssClass('myCustomCssClass');

Custom classes allow styling each node in a different way

[josdejong]

Thanks for your PR @maestr0 . The upgrade to gulp 4 is great, thanks for that. I can accept that PR right away though I prefer an exact type 4.0.0 instead of ^4.0.0. Is it possible for you to create a separate PR for this whcih we can immediately merge? (Current PR contains two totally different features)

As for the custom css classes: I really like your idea to allow for custom classes, that will be quite powerful in allowing custom styling and matching. I would love to discuss a different approach for this though. So far the public API of JSONEditor doesn't contain the internal nodes or their methods. Instead of exposing a method on the internal nodes, we could take a similar approach as the onEditable and onEvent callbacks (see docs), a declarative approach. Usage of a onClassName variant of these options could look something like:

const options = {
  onClassName = function (node) {
    const field = node.field
    const value = node.value
    const path = node.path // array with strings and numbers

    if (last(path) === 'customer') {
      return 'my-customer-class'
    }

    return undefined
  }
}
const editor = new JSONEditor(container, options, json)

What do you think?

[maestr0]

Ok, I will PR the gulp changes separately and implement the css class changes in the way you suggested.

[josdejong]

Awesome! Looking forward to it.

[meirotstein]

@josdejong I came across this PR and was thinking that maybe it will be better to go for a more general purpose solution that will allow adding additional attributes and hooks to the node dom element, something like:

const options = {
  onDomField = function (node, domField) {
    const field = node.field
    const value = node.value
    const path = node.path // array with strings and numbers

    if (last(path) === 'customer' && domField) {
      domField.classList.add('my-customer-class');
    }

  }
}
const editor = new JSONEditor(container, options, json)

WDYT?

[maestr0]

I like this idea but domField may not be fully initialized at this point. A field and a value are two separate dom elements so we should pass an enclosing element to the function or have two separate functions for a field and a value.

I'm not that familiar with the code so I leave it to @josdejong

[josdejong]

It's an interesting and very powerful idea @meirotstein, thanks. The downside is that we're fully opening up the internal DOM, which requires knowledge on the users side about it and a lot of documentation on the side of the editor. Whilst I would love to have such a powerful API, I think the current implementation is not suitable for such hooks (The react based v6 is built with support for such hooks in mind, though it's far from ready).

So, in short, for how I prefer a simple to understand onClassName option.

[josdejong]

@maestr0 what do you think?

[maestr0]

@josdejong I agree that exposing the internal DOM is not the best idea in this case. It can lead to many issues. I will implement onClassName in a way you suggested. I could also add onClickValue and onClickKey callbacks.

[maestr0]

@josdejong this is essentially the implementation you suggested. I added an example of how to implement a JSON diff view using this new feature.

[josdejong]

Thanks @maestr0! The code looks good and straight forward at first sight, I will check it out and run it locally within a couple of days I expect.

[josdejong]

@maestr0 I've tested your PR, I really like your diffing example, that's really cool :)

I have some feedback points:

1. Can you change the signature

   onClassName: function(path, field, value)

   to

   onClassName: function({ path, field, value })

   That way, the API of this new function is more consistent with similar callbacks like onEditable and onEvent.

2. Right now, attaching a custom class name only works on creation of a node. When the node changes, I think onClassName should be triggered again since the path, field, or value can be changed. This is very similar to Customize object names 2 #617 which also needs to recursively run an update when something changes, I think you can copy the solution taken there.

3. Can you remove the debugger line in from 20_custom_css_style_for_nodes.html?

4. In the example, I think a tricky thing in findNodeInJson(json, path) is that the function mutates the input argument path. It would be safer to have it slice path instead of change by using shift.

5. We should add a short section in the docs/api.md describing this new feature. I can do this too if you want.

[maestr0]

1,3 and 4 done. Working on 2
Please do 5

[maestr0]

2 sort of works now but it's not completed yet. The problem is that currently there's no way to remove a class when a node has changed. I can only add a new class which is not what we want. I have to find a way to toggle classes or maybe return two values, classes to be added and classes to be removed.

[maestr0]

@josdejong how about something like this

Node.prototype._updateCssClassName = function () { 
  // set custom css classes
  if(this.dom.field
    && this.editor 
    && this.editor.options 
    && typeof this.editor.options.onClassName ==='function'
    && this.dom.tree){              
      const { removeClasses, addClasses } = this.editor.options.onClassName({ path: this.getPath(), field: this.field, value: this.value });
      util.addClassName(this.dom.tree, addClasses);        
      util.removeClassName(this.dom.tree, removeClasses)
  }
}

[josdejong]

Cool @maestr0, looks good!

Good point about being able to remove a class name again. I'm not really fond of having to specify classes to add and to remove (imperative approach). Would it be possible to simply completely replace the class names with the newly returned result from onClassName (declarative approach)? I think that is more robust and simpler from a user perspective.

[maestr0]

The problem with replacing all existing classes with ones returned from onClassName is that there is already a .jsoneditor-values class on a table (json node) element and removing it may break something.

We could always add jsoneditor-values class to whatever is returned from onClassName

[josdejong]

We could always add jsoneditor-values class to whatever is returned from onClassName

Yes indeed, I think we should do that.

[josdejong]

ping @maestr0

[maestr0]

@josdejong updated the PR

      util.removeAllClassNames(this.dom.tree);              
      const addClasses = this.editor.options.onClassName({ path: this.getPath(), field: this.field, value: this.value }) || "";      
      util.addClassName(this.dom.tree, "jsoneditor-values " + addClasses); 

[josdejong]

@maestr0 I've done some testing, it works like a charm now 👍

I was wondering about one thing: the example suggests that both editors dynamically highlight the changes between the two JSON documents. When I rename for example "number" in the left editor, it highlights it in the left editor but not in the right editor because that editor doesn't get any trigger. It may be tricky to get that working though. Do you have any ideas?

And one small practical thing: You've committed a new file yarn.lock. I don't use yarn so I'm quite sure it will not stay up to date. Can you remove this file from your PR again?

[maestr0]

@josdejong removed yarn.lock and updated the example. I didn't know there was a refresh() function on the editor object.

[josdejong]

I didn't know there was a refresh() function on the editor object.

Ah, good find 😎 I hadn't thought of that. The method refresh() is not listed in the public API because there was no need for it until now. I will describe it in the API.

[josdejong]

@maestr0 I've done some minor tweaks in the example (20), I hope you don't mind, see a7cc852, 20e42ab, and db9f548.

[josdejong]

The feature onClassName is now available in v5.28.0, thanks again @maestr0 , I really like it :)