trufflesuite / drizzle-react-components-legacy

A set of useful components for common dapp UI elements.
95 stars 70 forks source link

Configurable Form in ContractForm.js #86

Closed mwaeckerlin closed 5 years ago

mwaeckerlin commented 5 years ago

Allow ContractForms to delegate rendering of the form details, similar to #81. A user should have the possibility to render the form according to his needs.

Currently:

Probably a render prop could solve most if not all of these requirements.

mwaeckerlin commented 5 years ago
  1. The current default implementation does not clean the input fields after sending the data. I suppose that should be fixed too, @honestbonsai?
  2. I'll try to rewrite your code to use React.createRef() or react.useState() to bind form elements to states. Do you agree, @honestbonsai?

See:

honestbonsai commented 5 years ago

@mwaeckerlin

  1. If this issue is just about render props, please try to keep it to just that. Open a new issue for cleaning input fields.
  2. I would consider this out of scope. Please keep refactors separate from feature additions/bug fixes. If you would like to refactor, please open a new issue + PR.

Hooks are in React 16.8 and are actually quite new. Moving to hooks isn't a priority for this code base. Considering that we rely on external dependencies for this code base, it's possible our users might not have even updated to 16.8. Once again, this is out of scope of this discussion. Please keep this issue only for render props.

I appreciate the enthusiasm and work you have done for this codebase. Keeping PRs and issues small will help us review and discuss it easier.

mwaeckerlin commented 5 years ago

@honestbonsai, only forward rendering is basically a 1-liner. But I am thinking and experimenting on what kind of data should be shared to make sense for reusing parts of the ContractForm in the render function.

That's why at least the second of my two points goes hand in hand with the basic render-prop-feature.

I googled around and found the hooks to be a nice way to easily share some basic data. I am still experimenting, but I already refactored to use hooks with the following big advantages:

That not only keeps the code smaller (better maintainable), but the reference could be a way to transfer data between render function and ContractForm.

Only «drawback»: Requirement for react-version increases from at least 15.4.2 to minimum 16.8.0. IMHO, that's acceptable for the advantages.

But:I am not yet satisfied with my current solution and still working on it, so things may change.

My main point: Some refactoring is needed, or at least helpful, to share data , that's why I would prefer to do it in one ticket.

mwaeckerlin commented 5 years ago

Some of my current investigations:

That's why I think about what data should be shared how.

BTW, @honestbonsai: ContractForm can pass data to the render-function's arguments, but is there a way for the render-function to return data to the ConractForm? I did not find any information on this yet.

Other than in ContractData, here rendering is only the first halve of the story, after rendering the form, there is the execution of the form, evaluation of user input and reaction on the button in the form. Therefore it is another situation.

My current approach:

  1. create the references in ContractForm (React.createRef())
  2. pass them in a map to the render function: inputs[name] = React.createRef()
  3. pass this.handleSubmit as callback to the render function

Note: That's still not the final solution, just the current status of my tests.

Another option would be, to pass this of ContractForm as argument to the render function, but that's not very smart.

What do you think, @honestbonsai, would you prefer another approach? Do you have better ideas?

mwaeckerlin commented 5 years ago

@honestbonsai, it works very well with the new ref-property. So this change is a must for my addition.

Currently, I am thinking about how to solve the following problem, perhaps you have a good idea, @honestbonsai:

How to handle this case: sendArgs requires a value and that value depends on a form input field?

In my example case, I have contract method that sends an amound of ether and the amount is entered in a form field.

The simplest approach does not work, because this of the ContractForm to be created is not accessible, so something like this does not work:

        <ContractForm
          contract="Sample"
          method="sendEther"
          sendArgs={{ value: this.inputs.amount.current.value }}
          render={(inputs, handleSubmit) => (
            <SendEther inputs={inputs} handleSubmit={handleSubmit} />
          )}
honestbonsai commented 5 years ago

Closed by #95 #96