Informed package refactor for input components

[Starotitorov]

This issue is for the following packages:

[x] venia-concept
[ ] pwa-buildpack
[ ] peregrine
[ ] pwa-devdocs
[ ] upward-js
[ ] upward-spec

This issue is a:

[ ] Bug
[ ] Feature suggestion
[ ] Documentation issue
[x] Tech Debt

Environment

Question	Answer
Magento version
Operating System + version
node.js version (node -v)
npm version (npm -v)

Description

In our application we decided to use informed package for managing forms. Informed uses Context API to store field values and other information such as errors, touched etc. In our common Input component we store value in local state, but we should do not do this, because informed is responsible for storing field values and handling changes in form state, our current implementation causes duplication of state. One of our bugs #416 with handling initial values inside Input component is the effect of this architecture mistake.

Expected result:

Field value should be handled by informed package.

Possible solutions:

We can wrap our Input component in asField HOC provided by informed and use BasicText component instead of Text. https://joepuzzo.github.io/informed/?selectedKind=CustomInputs&selectedStory=Creating%20Custom%20Inputs&full=0&addons=0&stories=1&panelRight=0&addonPanel=REACT_STORYBOOK%2Freadme%2Fpanel

[ericerway]

Updated with more descriptive title for issue. Thanks!

[pcvonz]

I'd like to take a crack at this. @JStein92 and I worked in a few places right as we made the commitment to use Informed.

[dmtrkad]

@pcvonz , Hey Paul, Let's wait for approval from @ericerway and @zetlen to continue working on it and sync up with changes we got to exclude double work effort from each side.

[pcvonz]

@dmtrkad Ok, sounds good! I'll find something else to work on in the meantime.

[jimbo]

Hi everyone. I'd like to chime in and provide some context.

The description of this issue is correct.

We're using informed to manage form and field state. Fields should be rendered inside a Form element, which will store form values and other state. This library creates event listeners and updates state automatically, so our field components should be stateless (generally).

This wasn't an architecture mistake, though; it was just a timing conflict.

I tried to have forms and field components in place before anyone was tasked with a user story that involved them, in order to avoid wasting anyone's time and effort. But the PR in which I introduced informed, #226, took a month to get merged, and during that time Paul and Jonathan were working on account creation and ended up creating the Input component.

We only need custom components to render Venia-themed informed components.

In a recent PR, I rewrote our Select component. Instead of being a fully custom implementation of Peregrine's List, it's now a simple wrapper around the Select component from informed. We probably need equivalents for Text, TextArea, Radio, and Checkbox. Application of classnames and restProps is really all these wrappers need to do.

[jimbo]

I understand that PRs have already been opened to resolve this issue. Hopefully my comments above will provide some insight as to what I'll be looking for.

@Starotitorov Yes, informed provides the asField HOC if you need to make a custom field. In our case, I don't think we need to customize the underlying inputs, so we should avoid the HOC and simply wrap the component (e.g., Text).

[Starotitorov]

@jimbo In the official documentation we have an example of input that renders validation error https://joepuzzo.github.io/informed/?selectedKind=CustomInputs&selectedStory=Creating%20Custom%20Inputs&full=0&addons=0&stories=1&panelRight=0&addonPanel=REACT_STORYBOOK%2Freadme%2Fpanel . I guess in our project we need something like this, so we need asField hoc. Text is just a BasicText component wrapped with asField hoc.

[Starotitorov]

@jimbo , I agree it is not a good idea to use simple html input, we are going to use BasicText.

[Starotitorov]

Also, in our common Input component we are going to add reset button to clear input value when use press it, so we definitely need this hoc.

[awilcoxa]

closing issue as related PRs were closed, it does not appear that the team is planning any additional work against this issue.