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

[UI Framework] [K7] Refactor form component interfaces #13493

Merged

Conversation

Projects
None yet
2 participants
@cjcenizal
Copy link
Contributor

commented Aug 13, 2017

  • 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 cjcenizal requested a review from snide Aug 13, 2017

@cjcenizal cjcenizal force-pushed the cjcenizal:k7/form-component-interfaces branch from af27e52 to 01b0224 Aug 13, 2017

@cjcenizal

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2017

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

@snide
Copy link
Contributor

left a comment

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.

image

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

image

box-shadow:
0 2px 2px -1px rgba(0, 0, 0, 0.1),
0 0 0 1px rgba(0,0,0,0.08),
inset -400px 0 0 0 $kuiFormBackgroundColor;

This comment has been minimized.

Copy link
@snide

snide Aug 14, 2017

Contributor

inset should use $kuiFormMaxWidth

@snide

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2017

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 added some commits Aug 11, 2017

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.
Update form examples to demonstrate all controls.
- Fix help text and error text spacing regression.
- Refine KuiCheckbox interface.

@cjcenizal cjcenizal force-pushed the cjcenizal:k7/form-component-interfaces branch from a799370 to bdc1134 Aug 14, 2017

@cjcenizal

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2017

@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.

image

@snide

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2017

@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

snide approved these changes Aug 14, 2017

value={value}
{...rest}
/>
<KuiFormControlLayout

This comment has been minimized.

Copy link
@snide

snide Aug 14, 2017

Contributor

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.

This comment has been minimized.

Copy link
@snide

snide Aug 14, 2017

Contributor

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?

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 15, 2017

Author Contributor

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! 😄

@cjcenizal cjcenizal merged commit ca6ecd2 into elastic:k7-ui-framework Aug 15, 2017

1 check passed

CLA Commit author has signed the CLA
Details

@cjcenizal cjcenizal deleted the cjcenizal:k7/form-component-interfaces branch Aug 15, 2017

cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Aug 16, 2017

[UI Framework] [K7] Refactor form component interfaces (elastic#13493)
* Add htmlIdGeneratorFactory service.

* 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.

* Refine isInvalid and error interface for FormRow and Form.

* Return child element without wrapper from KuiFormControlLayout if no icon is specified.

* Override form control invalid state with focus state.

* Update form examples to demonstrate all controls.
- Fix help text and error text spacing regression.
- Refine KuiCheckbox interface.

* Rename KuiCheckbox to KuiCheckboxGroup and remove top padding on first item.

* Refine KuiSwitch interface.

cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Aug 26, 2017

[UI Framework] [K7] Refactor form component interfaces (elastic#13493)
* Add htmlIdGeneratorFactory service.

* 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.

* Refine isInvalid and error interface for FormRow and Form.

* Return child element without wrapper from KuiFormControlLayout if no icon is specified.

* Override form control invalid state with focus state.

* Update form examples to demonstrate all controls.
- Fix help text and error text spacing regression.
- Refine KuiCheckbox interface.

* Rename KuiCheckbox to KuiCheckboxGroup and remove top padding on first item.

* Refine KuiSwitch interface.

cjcenizal added a commit that referenced this pull request Sep 19, 2017

[UI Framework] [K7] Refactor form component interfaces (#13493)
* Add htmlIdGeneratorFactory service.

* 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.

* Refine isInvalid and error interface for FormRow and Form.

* Return child element without wrapper from KuiFormControlLayout if no icon is specified.

* Override form control invalid state with focus state.

* Update form examples to demonstrate all controls.
- Fix help text and error text spacing regression.
- Refine KuiCheckbox interface.

* Rename KuiCheckbox to KuiCheckboxGroup and remove top padding on first item.

* Refine KuiSwitch interface.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.