Update Saved Object edit form with new components.

[cjcenizal]

CC @alt74 @uboness

I took some fields out of the below screenshots to fit everything on the screen.

Before

After

[BigFunger]

You made a comment about this format here. Where did this style come from? ie:

<tag
   attr="val"
>
content
</tag>

I see that it was added to the style guide, but it also seems to have been added by you. ;)

[cjcenizal]

Haha yeah that was me. I'll add further explanation to the style guide about it. I've found it to be useful for a few reasons...

Tag alignment

When I'm scanning markup, it's easier for me to discern the parent child relationship between the enclosing tags and the content when the tags share the same indentation, and the child is indented one level deeper. Conceptually, this makes sense to me too, since the enclosing tags become a parent node and the text content becomes a child node. So the markup mirrors the DOM a little more clearly.

Bracket alignment

When there are multiple attributes in the opening tag, having the closing bracket aligned to the same depth as the opening bracket helps me visually match them and group them. For example, I might scan and think content is an attribute if the closing bracket is on the same line:

<tag
  attr="val">
  content
</tag>

This might only make sense to me, but I also think of this way of indenting brackets as similar to the way you would indent the opening and closing braces and brackets in object and array definitions. They have the same indentation to form a visual alignment, which helps visually enclose their content:

const object = {
  'a': {
  },
  'b': [
    0,
    1,
    2,
  ],
};

Separation between tag and attributes

By putting the attributes on the same line it's easy for me to visually separate them from the tag. This is important because the tag defines the base identity of the element, but the attributes are used to customize it and define its role in the UI.

Easier attribute comparison

When there are a number of elements with similar, yet slightly different attributes, having each attribute on its own row makes it easier for me to scan, find the pattern, and discern differences.

<button
  class="kuiButton kuiButton--basic"
  ng-click="edit(service, item)"
  aria-label="Edit"
>
  <span
    aria-hidden="true"
    class="kuiButton__icon kuiIcon fa-pencil"
  ></span>
</button>

<button
  class="kuiButton kuiButton--basic"
  ng-click="open(item)"
  aria-label="View"
>
  <span
    aria-hidden="true"
    class="kuiButton__icon kuiIcon fa-eye"
  ></span>
</button>

In this example, I need to scan and check that:

1. Both buttons follow the same general structure: they're both default icon buttons.
2. Both buttons have aria-label attributes.
3. The aria-label attribute is different for each and has the correct value.
4. Both buttons contain icons.
5. Both icons have aria-hidden="true" attributes.
6. Each icon is a different (and correct) kind of icon.

This would be much harder for me to do if they were formatted like this:

<button class="kuiButton kuiButton--basic" ng-click="edit(service, item)" aria-label="Edit">
  <span aria-hidden="true" class="kuiButton__icon kuiIcon fa-pencil"></span>
</button>

<button class="kuiButton kuiButton--basic" ng-click="open(item)" aria-label="View">
  <span aria-hidden="true" class="kuiButton__icon kuiIcon fa-eye"></span>
</button>

I think the difficulty of scanning this kind of markup is that you have to scan right, jump to the next line, start at the left side, and continue. It's sort of like the reason we write things in list form -- because lists are easier to scan than long comma-separated sentences. Also, I think the shape of the opening tag becomes easier to identify. So I don't need to actually read and compare every attribute inside of it. Once it has a general shape, it's easier to see where the shape changes in the other opening tags that I'm comparing it against.

[BigFunger]

I totally agree that the vertical listing of attributes makes sense, and 100% support that. I really don't like the floating > of the opening tag...

...Conceptually, this makes sense to me too, since the enclosing tags become a parent node and the text content becomes a child node.
For example, I might scan and think content is an attribute if the closing bracket is on the same line

I conceptualize it in the opposite way. This really only becomes an issue when a tag contains simple text as its content. In that case, I see the attributes of a tag and the contents of a tag to all describe what that tag is supposed to represent, and think it makes sense to have them all justified.

The > isn't a 'tag' it's part of the definition of the opening tag of the element.

This might only make sense to me, but I also think of this way of indenting brackets as similar to the way you would indent the opening and closing braces and brackets in object and array definitions.

Your pattern, if applied to object and array declarations, would be more akin to this. For clarity, I would not suggest this either.

const object = {
  'a':
  {
  },
  'b': 
  [
    0,
    1,
    2,
  ],
};

[cjcenizal]

A better example just occurred to me. I used to write a lot of Jade and that must be where I picked up this pattern. Here's how Jade (now known as Pug) structures its tags:

input(
  type='checkbox'
  name='agreement'
  checked
)

Can you see how this structure is much more analogous to the way I'm proposing? The tag is almost like a function, and the attributes are what gets passed into it. The brackets act to encapsulate the attributes... due to the way HTML works though, your opening bracket comes before the "function name" instead of after.

[cjcenizal]

One last objective argument for the dangling bracket is that it's easier to add/remove/shuffle attributes. You won't need to mess with the closing bracket to make sure it's on the last attribute.

[w33ble]

I notice the background in the editor is now white, while the background for everything else is a light gray. Is this intentional? Are you planning to update all the other pages to match?

[cjcenizal]

@w33ble Yep, we're going to update all of Kibana to have a white background.

[cjcenizal]

I resolved some merge conflicts and force-pushed FYI.

[cjcenizal]

This passes the functional tests locally.

[w33ble]

Why the de-emphasis on the warning dialog? Users should, generally speaking, not be in this screen, so that broad warning is helpful.

---

This version was much more apparent. I immediately see that the text in that warning is important

---

Now, me being the non-reading user that I am, I don't even see the text at the top, or notice the tiny icon there. There's also no indication that the text under that little heading is a warning message of any kind. It seems like a regression.

---

Even just a small amount of contrast here would help tremendously (here, a background color and a small amount of vertical padding):

[w33ble]

I guess the white on white conversation is off the table at this point... so aside from the regression on the warning dialog, this PR seems fine.

[cjcenizal]

@w33ble Great catch, and thanks for the suggestion. I added a component called InfoPanel to the UI Framework to present content with this yellow background and updated the form with it:

[w33ble]

What about warning them against decisions they will certainly regret?

[w33ble]

Looks like you pulled the color from my screenshot. I'm no designer, and I know nothing about color theory, I just moved a slider until I found a color that looked good enough to me. I'm fine with leaving it this way, since I think it looks good enough, but just wanted to give you a little warning...

[cjcenizal]

Haha thanks... I think it's good for now. We'll adjust later.

[w33ble]

Still not a fan of all that white on white, but this PR LGTM!

[BigFunger]

I still don't like the HTML styling, but this is a stopgap PR anyway.

LGTM