Merge pull request #661 from prymitive/refactor-filter-input

refactor(ui): use button instead of a badge for filter elements

ui/src/App.scss

@@ -38,3 +38,6 @@ $nav-tabs-link-active-bg: #fff;
 .cursor-pointer {
   cursor: pointer;
 }
+.cursor-text {
+  cursor: text;
+}

ui/src/Common/Colors.js

@@ -1,17 +1,7 @@
 // fallback class for labels
-const DefaultLabelClass = "badge-warning components-label-dark";
-
-// labels configured as static will have badge-${this class}
-const StaticColorLabelClass = "badge-info components-label-dark";
-
-// alertname label will use this one
-const AlertNameLabelClass = "badge-dark components-label-dark";
-
-// alert state label will use one of those, based on the value
-const StateLabelClassMap = Object.freeze({
-  active: "badge-danger components-label-dark",
-  suppressed: "badge-success components-label-dark",
-  unprocessed: "badge-secondary components-label-bright"
+const DefaultLabelClassMap = Object.freeze({
+  badge: "badge-warning components-label-dark",
+  btn: "btn-warning components-label-dark"
 });
 
 // same but for borders
@@ -27,11 +17,27 @@ const BackgroundClassMap = Object.freeze({
   unprocessed: "bg-secondary"
 });
 
+const StaticColorLabelClassMap = Object.freeze({
+  badge: "badge-info components-label-dark",
+  btn: "btn-info components-label-dark"
+});
+
+const AlertNameLabelClassMap = Object.freeze({
+  badge: "badge-dark components-label-dark",
+  btn: "btn-dark components-label-dark"
+});
+
+const StateLabelClassMap = Object.freeze({
+  active: "danger",
+  suppressed: "success",
+  unprocessed: "secondary"
+});
+
 export {
-  DefaultLabelClass,
-  StaticColorLabelClass,
-  AlertNameLabelClass,
-  StateLabelClassMap,
+  DefaultLabelClassMap,
+  StaticColorLabelClassMap,
+  AlertNameLabelClassMap,
   BorderClassMap,
-  BackgroundClassMap
+  BackgroundClassMap,
+  StateLabelClassMap
 };

ui/src/Components/Labels/BaseLabel/index.js

@@ -3,10 +3,10 @@ import PropTypes from "prop-types";
 
 import { AlertStore } from "Stores/AlertStore";
 import {
-  StaticColorLabelClass,
-  StateLabelClassMap,
-  DefaultLabelClass,
-  AlertNameLabelClass
+  StaticColorLabelClassMap,
+  DefaultLabelClassMap,
+  AlertNameLabelClassMap,
+  StateLabelClassMap
 } from "Common/Colors";
 import { QueryOperators, FormatQuery, StaticLabels } from "Common/Query";
 
@@ -22,15 +22,17 @@ class BaseLabel extends Component {
     value: PropTypes.string.isRequired
   };
 
-  getClassAndStyle(name, value, extraClass = "") {
+  getClassAndStyle(name, value, extraClass, baseClass) {
     const { alertStore } = this.props;
 
+    const elementType = baseClass || "badge";
+
     const data = {
       style: {},
       className: "",
       baseClassNames: [
         "components-label",
-        "badge",
+        elementType,
         "text-nowrap",
         "text-truncate",
         "mw-100"
@@ -39,11 +41,15 @@ class BaseLabel extends Component {
     };
 
     if (name === StaticLabels.AlertName) {
-      data.colorClassNames.push(AlertNameLabelClass);
+      data.colorClassNames.push(AlertNameLabelClassMap[elementType]);
     } else if (name === StaticLabels.State) {
-      data.colorClassNames.push(StateLabelClassMap[value] || DefaultLabelClass);
+      data.colorClassNames.push(
+        StateLabelClassMap[value]
+          ? `${elementType}-${StateLabelClassMap[value]}`
+          : DefaultLabelClassMap[elementType]
+      );
     } else if (alertStore.settings.values.staticColorLabels.includes(name)) {
-      data.colorClassNames.push(StaticColorLabelClass);
+      data.colorClassNames.push(StaticColorLabelClassMap[elementType]);
     } else {
       const c = alertStore.data.getColorData(name, value);
       if (c) {
@@ -62,12 +68,12 @@ class BaseLabel extends Component {
         );
       } else {
         // if not fall back to class
-        data.colorClassNames.push(DefaultLabelClass);
+        data.colorClassNames.push(DefaultLabelClassMap[elementType]);
       }
     }
     data.className = `${[...data.baseClassNames, ...data.colorClassNames].join(
       " "
-    )} ${extraClass}`;
+    )} ${extraClass || ""}`;
 
     return data;
   }

ui/src/Components/Labels/BaseLabel/index.scss

@@ -12,24 +12,45 @@
 
 .components-label-bright {
   color: $black;
+  &:hover {
+    color: $black;
+  }
+
   &.components-label-name,
   .components-label-name {
     color: lighten($black, 20%);
+    &:hover {
+      color: lighten($black, 20%);
+    }
   }
   &.components-label-value,
   .components-label-value {
     color: $black;
+    &:hover {
+      color: $black;
+    }
   }
 }
 
 .components-label-dark {
   color: $white;
+  &:hover {
+    color: $white;
+  }
+
   &.components-label-name,
   .components-label-name {
     color: darken($white, 10%);
+    &:hover {
+      color: darken($white, 10%);
+    }
   }
+
   &.components-label-value,
   .components-label-value {
     color: $white;
+    &:hover {
+      color: $white;
+    }
   }
 }

ui/src/Components/Labels/BaseLabel/index.test.js

@@ -4,9 +4,9 @@ import { shallow } from "enzyme";
 
 import { AlertStore } from "Stores/AlertStore";
 import {
-  StaticColorLabelClass,
-  DefaultLabelClass,
-  AlertNameLabelClass,
+  StaticColorLabelClassMap,
+  DefaultLabelClassMap,
+  AlertNameLabelClassMap,
   StateLabelClassMap
 } from "Common/Colors";
 import { BaseLabel } from ".";
@@ -36,62 +36,68 @@ const FakeBaseLabel = (name = "foo", value = "bar") => {
 };
 
 describe("<BaseLabel />", () => {
-  it("static label uses StaticColorLabelClass", () => {
+  it("static label uses StaticColorLabelClassMap.badge", () => {
     alertStore.settings.values.staticColorLabels = ["foo", "job", "bar"];
     const tree = FakeBaseLabel();
-    expect(tree.find(".components-label").hasClass(StaticColorLabelClass)).toBe(
-      true
-    );
+    expect(
+      tree.find(".components-label").hasClass(StaticColorLabelClassMap.badge)
+    ).toBe(true);
   });
 
-  it("non-static label doesn't use StaticColorLabelClass", () => {
-    alertStore.settings.values.staticColorLabels = [];
+  Object.entries(StaticColorLabelClassMap).map(([key, val]) =>
+    it(`non-static label doesn't use StaticColorLabelClassMap.${key}`, () => {
+      alertStore.settings.values.staticColorLabels = [];
+      const tree = FakeBaseLabel();
+      expect(tree.find(".components-label").hasClass(val)).toBe(false);
+    })
+  );
+
+  it("label with no special color information should use DefaultLabelClassMap.badge", () => {
     const tree = FakeBaseLabel();
-    expect(tree.find(".components-label").hasClass(StaticColorLabelClass)).toBe(
-      false
-    );
+    expect(
+      tree.find(".components-label").hasClass(DefaultLabelClassMap.badge)
+    ).toBe(true);
   });
 
-  it("label with no special color information should use DefaultLabelClass", () => {
-    const tree = FakeBaseLabel();
-    expect(tree.find(".components-label").hasClass(DefaultLabelClass)).toBe(
-      true
-    );
-  });
-
-  it("alertname label should use AlertNameLabelClass", () => {
+  it("alertname label should use AlertNameLabelClassMap.badge", () => {
     const tree = FakeBaseLabel("alertname", "foo");
-    expect(tree.find(".components-label").hasClass(AlertNameLabelClass)).toBe(
-      true
-    );
+    expect(
+      tree.find(".components-label").hasClass(AlertNameLabelClassMap.badge)
+    ).toBe(true);
   });
 
   it("@state=active label should use StateLabelClassMap.active class", () => {
     const tree = FakeBaseLabel("@state", "active");
     expect(
-      tree.find(".components-label").hasClass(StateLabelClassMap.active)
+      tree
+        .find(".components-label")
+        .hasClass(`badge-${StateLabelClassMap.active}`)
     ).toBe(true);
   });
 
   it("@state=suppressed label should use StateLabelClassMap.suppressed class", () => {
     const tree = FakeBaseLabel("@state", "suppressed");
     expect(
-      tree.find(".components-label").hasClass(StateLabelClassMap.suppressed)
+      tree
+        .find(".components-label")
+        .hasClass(`badge-${StateLabelClassMap.suppressed}`)
     ).toBe(true);
   });
 
   it("@state=unprocessed label should use StateLabelClassMap.unprocessed class", () => {
     const tree = FakeBaseLabel("@state", "unprocessed");
     expect(
-      tree.find(".components-label").hasClass(StateLabelClassMap.unprocessed)
+      tree
+        .find(".components-label")
+        .hasClass(`badge-${StateLabelClassMap.unprocessed}`)
     ).toBe(true);
   });
 
-  it("@state with unknown label should use DefaultLabelClass", () => {
+  it("@state with unknown label should use DefaultLabelClassMap.badge", () => {
     const tree = FakeBaseLabel("@state", "foobar");
-    expect(tree.find(".components-label").hasClass(DefaultLabelClass)).toBe(
-      true
-    );
+    expect(
+      tree.find(".components-label").hasClass(DefaultLabelClassMap.badge)
+    ).toBe(true);
   });
 
   it("style prop on a label included in staticColorLabels should be empty", () => {

ui/src/Components/Labels/FilterInputLabel/index.css

@@ -1,34 +0,0 @@
-.badge.components-filteredinputlabel {
-  /* make fonts bigger */
-  font-size: 90%;
-
-  /* fix align after text-truncate */
-  vertical-align: middle;
-
-  padding-top: 0.15rem;
-  padding-bottom: 0.15rem;
-  padding-left: 0.15rem;
-
-  min-height: 1.5rem;
-
-  /* ensure there's some space between filters
-     if there are multiple rows in the input */
-  margin-top: 1px;
-  margin-bottom: 1px;
-}
-
-.badge.components-filteredinputlabel > .badge,
-.badge.components-filteredinputlabel > svg {
-  /* match outer badge font size */
-  font-size: 90%;
-  line-height: 100%;
-
-  /* we have badge inside a badge, make the inner one a little brighter
-     in case both use same color classes */
-  filter: brightness(115%);
-}
-
-.badge.components-filteredinputlabel > .close {
-  /* make close button bigger, to match font size of the badge */
-  font-size: 100%;
-}

ui/src/Components/Labels/FilterInputLabel/index.js

@@ -14,7 +14,7 @@ import { QueryOperators } from "Common/Query";
 import { TooltipWrapper } from "Components/TooltipWrapper";
 import { BaseLabel } from "Components/Labels/BaseLabel";
 
-import "./index.css";
+import "./index.scss";
 
 const FilterInputLabel = observer(
   class FilterInputLabel extends BaseLabel {
@@ -48,7 +48,8 @@ const FilterInputLabel = observer(
       let cs = this.getClassAndStyle(
         filter.matcher === QueryOperators.Equal ? filter.name : "",
         filter.matcher === QueryOperators.Equal ? filter.value : "",
-        "components-filteredinputlabel"
+        "components-filteredinputlabel btn-sm border-0",
+        "btn"
       );
 
       const showCounter =
@@ -59,13 +60,14 @@ const FilterInputLabel = observer(
       const rootClasses = filter.applied
         ? cs.className
         : [
-            "badge-secondary components-filteredinputlabel",
+            "btn-secondary btn-sm border-0 components-filteredinputlabel",
             ...cs.baseClassNames
           ].join(" ");
 
       return (
-        <span
-          className={`${rootClasses} d-inline-flex flex-row`}
+        <button
+          type="button"
+          className={`${rootClasses} d-inline-flex flex-row align-items-center`}
           style={filter.applied ? cs.style : {}}
         >
           {filter.isValid ? (
@@ -76,21 +78,20 @@ const FilterInputLabel = observer(
                 </span>
               ) : null
             ) : (
-              <span className="badge">
-                <FontAwesomeIcon icon={faSpinner} spin />
-              </span>
+              <FontAwesomeIcon icon={faSpinner} spin />
             )
           ) : (
-            <span className="badge text-danger">
-              <FontAwesomeIcon icon={faExclamationCircle} />
-            </span>
+            <FontAwesomeIcon
+              icon={faExclamationCircle}
+              className="text-danger"
+            />
           )}
           <TooltipWrapper
             title="Click to edit this filter"
-            className="my-auto mw-100 text-nowrap text-truncate"
+            className="my-auto text-nowrap text-truncate align-text-bottom"
           >
             <RIEInput
-              className="ml-1"
+              className="cursor-text ml-1"
               defaultValue=""
               value={filter.raw}
               propName="raw"
@@ -98,21 +99,20 @@ const FilterInputLabel = observer(
               classEditing="py-0 border-0 bg-light"
             />
           </TooltipWrapper>
-          <button
-            type="button"
-            className="close ml-1"
+          <span
+            className="close ml-1 align-text-bottom cursor-pointer"
             style={filter.applied ? cs.style : {}}
             onClick={() => alertStore.filters.removeFilter(filter.raw)}
           >
             <span
               className={cs.colorClassNames
-                .filter(c => !c.match(/badge-/))
+                .filter(c => !c.match(/btn-|badge-/))
                 .join(" ")}
             >
               &times;
             </span>
-          </button>
-        </span>
+          </span>
+        </button>
       );
     }
   }

ui/src/Components/Labels/FilterInputLabel/index.scss

@@ -0,0 +1,45 @@
+@import "~bootswatch/dist/flatly/variables";
+
+.components-filteredinputlabel > .badge,
+.components-filteredinputlabel > svg {
+  /* match outer badge font size */
+  font-size: 100%;
+
+  /* we have badge inside a badge, make the inner one a little brighter
+     in case both use same color classes */
+  filter: brightness(115%);
+}
+
+button.btn.components-filteredinputlabel {
+  cursor: default;
+  margin-top: 0.125rem;
+  margin-bottom: 0.125rem;
+}
+
+button.components-label.btn {
+  &.btn-primary:hover {
+    background-color: $primary;
+  }
+  &.btn-secondary:hover {
+    background-color: $secondary;
+  }
+  &.btn-success:hover {
+    background-color: $success;
+  }
+  &.btn-info:hover {
+    background-color: $info;
+  }
+  &.btn-warning:hover {
+    background-color: $warning;
+  }
+  &.btn-danger:hover {
+    background-color: $danger;
+  }
+  &.btn-light:hover {
+    background-color: $light;
+  }
+  &.btn-dark:hover {
+    // same as in App.scss
+    background-color: #3b4247;
+  }
+}

ui/src/Components/Labels/FilterInputLabel/index.test.js

@@ -54,35 +54,35 @@ const ValidateOnChange = newRaw => {
 };
 
 describe("<FilterInputLabel /> className", () => {
-  it("unapplied filter with '=' matcher should use 'badge-secondary' class", () => {
-    ValidateClass("=", false, "badge-secondary");
+  it("unapplied filter with '=' matcher should use 'btn-secondary' class", () => {
+    ValidateClass("=", false, "btn-secondary");
   });
 
-  it("unapplied filter with any matcher other than '=' should use 'badge-secondary' class", () => {
+  it("unapplied filter with any matcher other than '=' should use 'btn-secondary' class", () => {
     for (const matcher of NonEqualMatchers) {
-      ValidateClass(matcher, false, "badge-secondary");
+      ValidateClass(matcher, false, "btn-secondary");
     }
   });
 
-  it("applied filter with '=' matcher and no color should use 'badge-warning' class", () => {
-    ValidateClass("=", true, "badge-warning");
+  it("applied filter with '=' matcher and no color should use 'btn-warning' class", () => {
+    ValidateClass("=", true, "btn-warning");
   });
 
-  it("applied filter with any matcher other than '=' and no color should use 'badge-warning' class", () => {
+  it("applied filter with any matcher other than '=' and no color should use 'btn-warning' class", () => {
     for (const matcher of NonEqualMatchers) {
-      ValidateClass(matcher, true, "badge-warning");
+      ValidateClass(matcher, true, "btn-warning");
     }
   });
 
-  it("applied filter included in staticColorLabels with '=' matcher should use 'badge-info' class", () => {
+  it("applied filter included in staticColorLabels with '=' matcher should use 'btn-info' class", () => {
     alertStore.settings.values.staticColorLabels = ["foo"];
-    ValidateClass("=", true, "badge-info");
+    ValidateClass("=", true, "btn-info");
   });
 
-  it("applied filter included in staticColorLabels with any matcher other than '=' should use 'badge-warning' class", () => {
+  it("applied filter included in staticColorLabels with any matcher other than '=' should use 'btn-warning' class", () => {
     alertStore.settings.values.staticColorLabels = ["foo"];
     for (const matcher of NonEqualMatchers) {
-      ValidateClass(matcher, true, "badge-warning");
+      ValidateClass(matcher, true, "btn-warning");
     }
   });
 });
@@ -171,7 +171,7 @@ describe("<FilterInputLabel /> onChange", () => {
         filter={alertStore.filters.values[0]}
       />
     );
-    const button = tree.find("button");
+    const button = tree.find(".close");
     button.simulate("click");
     expect(alertStore.filters.values).toHaveLength(1);
     expect(alertStore.filters.values).toContainEqual(

ui/src/Components/NavBar/FilterInput/__snapshots__/index.test.js.snap

@@ -21,7 +21,7 @@ exports[`<FilterInput /> matches snapshot on default render 1`] = `
       </svg>
     </span>
   </div>
-  <div class=\\"form-control p-1 components-filterinput\\">
+  <div class=\\"form-control components-filterinput\\">
     <div role=\\"combobox\\"
          aria-haspopup=\\"listbox\\"
          aria-owns=\\"react-autowhatever-1\\"

ui/src/Components/NavBar/FilterInput/index.css

@@ -1,6 +1,11 @@
 .form-control.components-filterinput {
   cursor: text;
   height: auto;
+
+  padding-top: 0.125rem;
+  padding-bottom: 0.125rem;
+  padding-left: 0.25rem;
+  padding-right: 0.25rem;
 }
 
 input.components-filterinput-wrapper {
@@ -11,3 +16,7 @@ input.components-filterinput-wrapper {
   padding: 0;
   vertical-align: middle;
 }
+
+.autosuggest {
+  min-height: 2rem;
+}

ui/src/Components/NavBar/FilterInput/index.js

@@ -144,7 +144,7 @@ const FilterInput = observer(
               </span>
             </div>
             <div
-              className="form-control p-1 components-filterinput"
+              className="form-control components-filterinput"
               onClick={event => {
                 this.onInputClick(this.inputStore.ref, event);
               }}