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

Fix for #738: Code for Accepting custom Value render method #819

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Fix for #738: Code for Accepting custom Value render method #819

wants to merge 5 commits into from

Conversation

123survesh
Copy link
Contributor

@123survesh 123survesh commented Oct 18, 2019

Hi @josdejong ,

I have written some code to accept a custom render method for value. I have tried to write it as close to the API format that you had put up in the issue page. I left out a few things that I didn't understand.

onRenderValue({ path, field, value, schema, onChange }) => { // I couldn't get schema so I am not sending schema in my code
  const html = document.createElement('INPUT')
  html.value = value
  html.onchange = function (event) {
    onChange(event.target.value)
  }
  
  return {
    html, 
    update: (value) => {
      html.value = value
    },
    destroy: () => { 
      // clean up any instantiated DOM stuff if needed
    } // I have not implemented update or destroy as I didn't know where to use them in the code and wanted to clarify it with you.
  }
}

Please do check and tell if I am going in the right direction or should I go with another approach to it. I am kind of new.

@josdejong
Copy link
Owner

Thanks @123survesh ! I will look into your PR soon, curious to see how far you are already :)

@josdejong
Copy link
Owner

This is a nice start Survesh!

  1. I think we should allow onRenderValue to return either null or a renderData object: in practice I think you sometimes want to create a custom value Like when the value contains a color you want to display a color picker, or for certain paths like customer.address you want to create a button to open the address on the map.

  2. Since _createDomValue is growing quite large now, it may help extracting functionality into helper methods _renderDefaultValue and _renderCustomValue

  3. instead of the this.customValueRenderFlag, you can store the renderData in the node, so you can (later on) call the .destroy and .update (or maybe we can call it .onUpdate).

  4. The JSON Schema is available on the node as this.schema, but of course only when a JSON Schema is defined.

  5. the destroy method can be needed when a custom value needs to do some cleanup of say globally created listeners or so. This method should be called from Node.hide(), which fully removes a node from the DOM.

  6. The update method on a custom node may be called when the JSON document is updated from outside, using the editor.set() or editor.update(). We could go two ways there: we could implement this change method, or alternatively simply destroy the current custom nodes and create new ones in case of a change.

  7. When working this feature out, are a lot of edge cases we need to check: when changing the JSON Schema, changing the type of a value from say a string to an array, dragging a node to a different location in the document, etc. It will not be easy I think.

@josdejong
Copy link
Owner

@123survesh have you seen my feedback points? What do you think about it?

@123survesh
Copy link
Contributor Author

Hi @josdejong ,
Sorry I didn't reply, I got held up with some stuff. I did read through the feedbacks. They answer my questions and gave me clarity as to how the update() and delete() will be used and what schema actually was.
I also understand from your feedback that there are a lot of edge cases where the rendered values might be updated and we have to cover them all.

For Point 2 in your feedback, I have a doubt,
As per my understanding, the createDomValue() method creates wrappers for the node based on its type. And the updateDomValue() method renders the data for that node based on its type. Shouldn't we split the updateDomValue() method instead?

I will start working on the issues in 2 or 3 days. Again I am sorry for not replying sooner.

@josdejong
Copy link
Owner

Survesh no need at all to apologize! There are no deadlines, it goes as fast as it goes and it's ready when it's ready :).

About Point 2: The method createDomValue will create the DOM for a value, and updateDomValue updates the DOM of a value that has been created before. We probably need modifications in both. Ideas for improvement of the code are always welcome, but let's be careful not to drag in a large refactoring rightaway and focus on custom value rendering in this PR. Al I meant with Point 2 is that it may be helpful to park part of the code inside createDomValue in a separate function to keep the whole readable. Feel free to decide whether that indeed helps not.

@123survesh
Copy link
Contributor Author

Ok @josdejong got it 🙂 , there are no hard deadlines.

Al I meant with Point 2 is that it may be helpful to park part of the code inside createDomValue in a separate function to keep the whole readable.

Ok I understand, I will see what I can do and I will get back to you if I have any doubts.

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.

None yet

2 participants