fix #7194 Editor errors on Visualize are broken

[ppisljar]

fix for #7194 - editor errors on visualize are broken

updated edtor.less file to show error icon in the same style as the buttons next to it.

[Bargs]

Functionally looks good to me, but let's take this as an opportunity to clean up the css based on @cjcenizal's guide: #7364

[ppisljar]

@cjcenizal could you please review what i did so far before i continue ...

• i renamed classes to follow your guide ... but i am not 100% i understood correctly:
  -- using camel case for component names
  -- using __ for subcomponents
  -- using -- for modifiers
• replaced some of the html element selectors with class name selectors

if i understood correctly i dont need to take bootstrap out now, we'll be doing this gradually ? (since quite a lot of styles in this file are bootstrap modifiers)

please be aware that this is still work in progress, but i would fell better if i would know i am going to the right dirrection.

[cjcenizal]

@ppisljar You nailed the new naming convention! Thanks for doing this. I think this is great in this PR, but in the future I think it's better to should hold off on these kinds of "mass-renaming" changes for now, for a couple reasons...

1. I'm concerned about introducing two naming conventions in a single codebase. I think this might make things more confusing for developers.
2. The ROI is pretty low, especially considering many of our styles need to be completely rethought as reusable UI components, which will require many class names and properties to change dramatically.

So I think it's OK if we stick with the existing kebab-case naming convention for components that already exist in Kibana and only apply the new naming convention for new components (either created in Kibana or in the new Kibana UI Framework I'm working on).

Instead of applying the new naming convention, I think more high-value changes to our CSS would be to reduce specificity (which has the added benefit of simplifying our styles). In this case, lines 11-35 of _editor.less contains numerous nested selectors, each of which introduces specificity. It's easy pretty difficult to read. I would explore whether this specificity is necessary for overriding the bootstrap navbar styles, or whether you can introduce classes with lower specificity that accomplish the same result.

Also, you're right that the migration away from bootstrap will be gradual, so you don't need to worry about that here.

[cjcenizal]

Technically this should be visEditorAgg__editorAdvancedToggle, but it really doesn't matter per my comment re mass name-changes.

[cjcenizal]

I opened up the inspector and checked out how the styles on line 11-35 of _editor.less were working.

I noticed a few interesting things:

1. The subnav class doesn't seem to be doing anything! It wasn't showing up as a selector in the inspector's styles tab, and I grepped kibana source and couldn't find it defined anywhere.
2. The over-reliance on the active class was causing problems, since it only added a couple styles to the element it was directly applied to, but was used to add styles implicitly to child elements.

So I untangled the nested selectors by creating a few unique classes, and using !important where necessary to override styles inherited by Bootstrap.

// _editor.less

.visEditor {
  .flex-parent();

  .btn-xs {
    line-height: 1.3;
  }

  /* ... */
}

  /**
   * 1. TODO: Override bootstrap styles. Remove !important once we're rid of bootstrap.
   */
  .visEditorSubNavLink {
    padding: 4px 10px 5px 10px !important; /* 1 */
    color: @kibanaGray2 !important; /* 1 */

    &.is-vis-editor-sub-nav-link-selected {
      border-bottom: 2px solid @kibanaGray2;
      color: @kibanaGray1;
      background-color: #f6f6f6;

      &:before {
        display: none;
      }

      &:hover {
        background-color: transparent;
      }
    }
  }

  /**
   * 1. TODO: Override bootstrap styles. Remove !important once we're rid of bootstrap.
   */
  .visEditorSubNavLink--danger {
    color: @vis-editor-navbar-error-state-color !important; /* 1 */
    background-color: @vis-editor-navbar-error-state-bg;

    &:hover {
      background-color: darken(@vis-editor-navbar-error-state-bg, 12%) !important; /* 1 */
    }
  }

<!-- sidebar.html -->

    <nav class="navbar navbar-default subnav">
      <div class="container-fluid">

        <!-- tabs -->
        <ul class="nav navbar-nav">
          <li ng-show="sidebar.showData">
            <a
              class="navbar-link visEditorSubNavLink"
              ng-class="{'is-vis-editor-sub-nav-link-selected': sidebar.section == 'data'}"
              ng-click="sidebar.section='data'"
            >
              @Data
            </a>
          </li>
          <li>
            <a
              class="navbar-link visEditorSubNavLink"
              ng-class="{'is-vis-editor-sub-nav-link-selected': sidebar.section == 'options'}"
              ng-click="sidebar.section='options'"
            >
              Options
            </a>
          </li>
        </ul>

        <!-- controls -->
        <ul class="nav navbar-nav navbar-right">
          <li ng-if="visualizeEditor.softErrorCount() > 0"
            disabled
            tooltip="{{ visualizeEditor.describeErrors() }}" tooltip-placement="bottom" tooltip-popup-delay="400" tooltip-append-to-body="1"
          >
            <a class="navbar-link visEditorSubNavLink visEditorSubNavLink--danger">
              <i class="fa fa-warning"></i>
            </a>
          </li>
          <li tooltip="Apply changes" tooltip-placement="bottom" tooltip-popup-delay="400" tooltip-append-to-body="1">
            <button class="btn-success navbar-btn-link"
              type="submit"
              ng-disabled="!vis.dirty">

              <i class="fa fa-play"></i>
            </button>
          </li>
          <li tooltip="Discard changes" tooltip-placement="bottom" tooltip-popup-delay="400" tooltip-append-to-body="1">
            <button class="btn-default navbar-btn-link"
              ng-disabled="!vis.dirty"
              ng-click="resetEditableVis()">

              <i class="fa fa-close"></i>
            </button>
          </li>
        </ul>
      </div>
    </nav>

As an added bonus, this fixes the styles, which I believe are currently "broken" (because Bootstrap is winning the specificity wars against some of the current selectors, so they're not all taking effect):

I can also make a PR into this branch if you want to see the diff more clearly. Just let me know.

[ppisljar]

@cjcenizal ok i reverted back to the old naming convention and i put in your suggestions on how to reduce specificity. i also removed navbar-link class as it seemed its not needed anymore.

please let me know if this is going into the right direction.

[cjcenizal]

Don't forget to revert these class names back to the original snake-case versions in all of the markup too.

[cjcenizal]

@ppisljar Left a couple comments! I can continue reviewing after they're addressed.

[ppisljar]

sorry seems i messed up the commit ... i think it should be ok now.

[cjcenizal]

jenkins, test this

[cjcenizal]

Small nit, the attributes should all be on their own lines, the > of the opening tag should be on its own line, and the text and closing tag should be on their own lines:

<a
  class="vis-editor-subnav-link"
  ng-class="{'is-vis-editor-sub-nav-link-selected': sidebar.section == 'data'}"
  ng-click="sidebar.section='data'"
>
  Data
</a>

[ppisljar]

I'm checking some of the other .html files and i cant find this anywhere (last > in a seperate line) ...
also my editor (webstorm) wont put > in line with <a (no indent) if i put it in a seperate line ....
would it be possible to configure .eseditorconfig and .eslint to follow this style ?

also could you update html style guide with this information:

[cjcenizal]

Hey Peter, the most recent version of the style guide exemplifies this in the examples. Should I update the descriptions to point this out more, or do you think this is enough?

This is a pretty recent change to the style guide. You can check out this PR for some context.

I'm using Sublime, so I'm not sure why WebStorm would be unhappy with that formatting... is this something you can look into fixing on your end? Let me know if you want to Zoom about this and tackle it together.

[ppisljar]

thanks CJ, im not sure how i managed to open the old style guide :) i managed to get webstorm working with it. I updated whole sidebar.html to follow the new style guide, can you please check if it looks ok ?

i still wonder if it would be possible to make a precommit check to enforce this (like we do with eslint) ?

[cjcenizal]

jenkins, test this

[cjcenizal]

I think this will be more maintainable if we separate the opening and closing tags from the content:

>
  Data
</a>

I think this consistency will let us change the content, e.g. adding more text, adding new elements, without having to add and remove new lines depending on how much content there is. What do you think?

[ppisljar]

i agree

[cjcenizal]

Oh wait, I found a couple typos here... on lines 21 and 22, it looks like ng-class and ng-click got turned into g-class and g-click.

[cjcenizal]

I just have 1 suggestion. Looks great overall! I agree that it would be great to get a linter to help us with these rules. I haven't been able to find one so far but I'll keep looking.

[cjcenizal]

LGTM! Just gotta make sure the tests pass.

[cjcenizal]

jenkins, test this