[UI Framework] [K7] Refactor form component interfaces

[cjcenizal]

• Add htmlIdGeneratorFactory service (unused for the moment)
• Extract KuiFormHelpText and KuiFormErrorText components.
• Add KuiFormLabel. Update form examples to emphasize the basic form components.
• Refine KuiFormRow, KuiSelect, and KuiFieldX component interfaces. …
• Add KuiFormControlLayout component for adding icons to controls.
• Make icon a concern of the field itself, not the KuiFormRow.
• Allow developer to specify icon for KuiFieldText and KuiFieldNumber.
• Hardcode icons for KuiSelect, KuiFieldPassword, and KuiFieldSearch components.
• Move validation from KuiFormRow to each individual component. 
• Create :invalid pseudo element selector.
• Create KuiValidatableControl component.

[cjcenizal]

Also, I'm thinking the focus state should override the invalid state. What do you think?

[snide]

This is great. Haven't dug too far into the code yet, but noticed a couple of small regressions...

Spacing in multi-line help/validation text got busted. Believe I had some & + code in here to remove top padding on non first-childs.

Also, looks like switch clicking busted. I think you might be missing the id/for attributes here. Should be an easy fix.

Also looks like dropdowns in your validation example are possibly passing a bad object array.

[snide]

inset should use $kuiFormMaxWidth

[snide]

I don't see anything major with the code. Your additions are great and the inputs are better detached now (especially with icons). It's less portable now (because you need the react for the state management, but that's an OK trade-off I guess).

[snide]

Also, small thing, but I'd include every form element in both the naked and form row views. Right now you're missing a couple in each example.

[cjcenizal]

@snide Thanks for the great review. I address your feedback, can you take another look? I also removed some padding between the checkbox group and the label, which made it inconsistent with the spacing in the other form rows.

[snide]

@cjcenizal Seeing a new issue with the checkboxes on the one where it's already checked (passed as prop).

Hehe. Soooo many different configs. I feel for ya.

[snide]

This looks great. Other than the checkbox bit, this is OK by me to merge. The rest is just me theorizing.

---

So I'll just bring this up because it's something I struggled with. Originally I had these raw components include FormRow in them by default. It made it easy to pass props down.

You essentially went half way there by making form control layout and making it only control the icon and layout of the naked input.

My question I guess is... what's so different from this and just putting FormRow in here directly? That way people wouldn't even need to mess with form rows, the usage code is much cleaner. Basically the way I had it originally (before I changed it at your rec) was that you'd just say something like...

<FieldSearch /> on its own.

This didn't pass labels, errors, invald states or icons. Icon was passed in the actual field_search.js as you're doing now through form_layout (before I just hadd it hardcoded into form_row in a similar forced fashion.

Essentially... do we need these things separated?

<Fieldsearch />

and

<FieldRow
  id="whatever"
  label="whatever"
>
  <Fieldsearch />
</FiedRow>

When we could just go ahead and put <FieldRow> inside of the component and just do this instead....

<Fieldsearch/> for naked.
<Fieldsearch id="whatever" label="whatever" /> for one with labeling...etc.

Seems like a whole lot less to remember, and the HTML will print out just as clean. I guess I just don't see the reason why we need the two components living as separate, usable things.

[snide]

Like, I understood it if you were just making it a fully naked input tag, but now it's got a necessary wrapper involved. At that point, just use the full wrapper and let people pass options?

[cjcenizal]

I'd characterize the difference between the two approaches as mostly philosophical, which at first can seem arbitrary, though in my experience the result in two very different maintenance scenarios.

To put on my engineer hat, we're embracing the single responsibility principle (SRP) by splitting up our form components into three categories: controls, layout, and row.

To adhere to SRP we want the single concern of each module to be clear to the user, and this concern should also contrast clearly against the concerns of other modules. Controls only surface the interactive parts of the form, layout decorates controls with other details like icons, and row coordinates how multiple {controls|layouts} appear together.

I think SRP has also created a set of components which compose well together, but don't depend on one another. For example, row presents controls well, but it can present anything well. We can toss anything in there and it will never break because it has no dependency upon what it presents. And controls are presented well by rows, but they have no dependency upon rows. We can make changes to one without really having to worry about it will affect the other, which is great for maintenance.

Code isn't maintainable without being understandable, and clear responsibilities should make this code easier to read and understand. Fewer responsibilities means fewer states and edge cases someone has to consider when making changes to code. For example, a <KuiFieldSearch /> which sometimes renders a <KuiFieldRow /> and sometimes renders a <KuiIcon /> means having to hold all of those different states in your head and consider how your change will affect each one.

I'm not sure if I'm answering your question well... let me know how I did! 😄