Merge pull request #367 from everett980/issue-361-and-360-improve-uiFind-interactions

add uiFind icons, scroll to first match

Author: everett980

packages/jaeger-ui/package.json

@@ -51,6 +51,7 @@
     "chance": "^1.0.10",
     "classnames": "^2.2.5",
     "combokeys": "^3.0.0",
+    "copy-to-clipboard": "^3.1.0",
     "cytoscape": "^3.2.1",
     "cytoscape-dagre": "^2.0.0",
     "d3-scale": "^1.0.6",
@@ -71,7 +72,6 @@
     "query-string": "^6.3.0",
     "raven-js": "^3.22.1",
     "react": "^16.3.2",
-    "react-copy-to-clipboard": "^5.0.1",
     "react-dimensions": "^1.3.0",
     "react-dom": "^16.3.2",
     "react-ga": "^2.4.1",

packages/jaeger-ui/src/components/TraceDiff/TraceDiffGraph/__snapshots__/drawNode.test.js.snap

@@ -25,6 +25,8 @@ exports[`drawNode diffNode renders as expected when props.a and props.b are the
             <CopyIcon
               className="DiffNode--copyIcon"
               copyText="serviceName operationName"
+              icon="copy"
+              placement="left"
               tooltipTitle="Copy label"
             />
           </td>
@@ -70,6 +72,8 @@ exports[`drawNode diffNode renders as expected when props.a and props.b are the
           <CopyIcon
             className="DiffNode--copyIcon"
             copyText="serviceName operationName"
+            icon="copy"
+            placement="left"
             tooltipTitle="Copy label"
           />
         </td>
@@ -116,6 +120,8 @@ exports[`drawNode diffNode renders as expected when props.a is 0 1`] = `
             <CopyIcon
               className="DiffNode--copyIcon"
               copyText="serviceName operationName"
+              icon="copy"
+              placement="left"
               tooltipTitle="Copy label"
             />
           </td>
@@ -181,6 +187,8 @@ exports[`drawNode diffNode renders as expected when props.a is 0 1`] = `
           <CopyIcon
             className="DiffNode--copyIcon"
             copyText="serviceName operationName"
+            icon="copy"
+            placement="left"
             tooltipTitle="Copy label"
           />
         </td>
@@ -242,6 +250,8 @@ exports[`drawNode diffNode renders as expected when props.a is less than props.b
             <CopyIcon
               className="DiffNode--copyIcon"
               copyText="serviceName operationName"
+              icon="copy"
+              placement="left"
               tooltipTitle="Copy label"
             />
           </td>
@@ -307,6 +317,8 @@ exports[`drawNode diffNode renders as expected when props.a is less than props.b
           <CopyIcon
             className="DiffNode--copyIcon"
             copyText="serviceName operationName"
+            icon="copy"
+            placement="left"
             tooltipTitle="Copy label"
           />
         </td>
@@ -368,6 +380,8 @@ exports[`drawNode diffNode renders as expected when props.a is more than props.b
             <CopyIcon
               className="DiffNode--copyIcon"
               copyText="serviceName operationName"
+              icon="copy"
+              placement="left"
               tooltipTitle="Copy label"
             />
           </td>
@@ -433,6 +447,8 @@ exports[`drawNode diffNode renders as expected when props.a is more than props.b
           <CopyIcon
             className="DiffNode--copyIcon"
             copyText="serviceName operationName"
+            icon="copy"
+            placement="left"
             tooltipTitle="Copy label"
           />
         </td>
@@ -494,6 +510,8 @@ exports[`drawNode diffNode renders as expected when props.b is 0 1`] = `
             <CopyIcon
               className="DiffNode--copyIcon"
               copyText="serviceName operationName"
+              icon="copy"
+              placement="left"
               tooltipTitle="Copy label"
             />
           </td>
@@ -559,6 +577,8 @@ exports[`drawNode diffNode renders as expected when props.b is 0 1`] = `
           <CopyIcon
             className="DiffNode--copyIcon"
             copyText="serviceName operationName"
+            icon="copy"
+            placement="left"
             tooltipTitle="Copy label"
           />
         </td>
@@ -615,6 +635,8 @@ exports[`drawNode diffNode renders as expected when props.isUiFindMatch is true
             <CopyIcon
               className="DiffNode--copyIcon"
               copyText="serviceName operationName"
+              icon="copy"
+              placement="left"
               tooltipTitle="Copy label"
             />
           </td>
@@ -660,6 +682,8 @@ exports[`drawNode diffNode renders as expected when props.isUiFindMatch is true
           <CopyIcon
             className="DiffNode--copyIcon"
             copyText="serviceName operationName"
+            icon="copy"
+            placement="left"
             tooltipTitle="Copy label"
           />
         </td>

packages/jaeger-ui/src/components/TraceDiff/TraceDiffGraph/drawNode.css

@@ -88,6 +88,7 @@ limitations under the License.
 .DiffNode--popover .DiffNode--copyIcon,
 .DiffNode:not(:hover) .DiffNode--copyIcon {
   color: transparent;
+  pointer-events: none;
 }
 
 /* Tweak the popover aesthetics - unfortunate but necessary */

packages/jaeger-ui/src/components/TracePage/ScrollManager.test.js

@@ -21,15 +21,14 @@ import ScrollManager from './ScrollManager';
 const SPAN_HEIGHT = 2;
 
 function getTrace() {
-  let nextSpanID = 0;
   const spans = [];
   const trace = {
     spans,
     duration: 2000,
     startTime: 1000,
   };
   for (let i = 0; i < 10; i++) {
-    spans.push({ duration: 1, startTime: 1000, spanID: nextSpanID++ });
+    spans.push({ duration: 1, startTime: 1000, spanID: i + 1 });
   }
   return trace;
 }
@@ -106,6 +105,9 @@ describe('ScrollManager', () => {
   });
 
   describe('_scrollToVisibleSpan()', () => {
+    function getRefs(spanID) {
+      return [{ refType: 'CHILD_OF', spanID }];
+    }
     let scrollPastMock;
 
     beforeEach(() => {
@@ -142,7 +144,7 @@ describe('ScrollManager', () => {
 
     it('skips spans that are out of view', () => {
       trace.spans[4].startTime = trace.startTime + trace.duration * 0.5;
-      accessors.getViewRange = jest.fn(() => [0.4, 0.6]);
+      accessors.getViewRange = () => [0.4, 0.6];
       accessors.getTopRowIndexVisible.mockReturnValue(trace.spans.length - 1);
       accessors.getBottomRowIndexVisible.mockReturnValue(0);
       manager._scrollToVisibleSpan(1);
@@ -154,18 +156,41 @@ describe('ScrollManager', () => {
     it('skips spans that do not match the text search', () => {
       accessors.getTopRowIndexVisible.mockReturnValue(trace.spans.length - 1);
       accessors.getBottomRowIndexVisible.mockReturnValue(0);
-      accessors.getSearchedSpanIDs = jest.fn(() => new Set([trace.spans[4].spanID]));
+      accessors.getSearchedSpanIDs = () => new Set([trace.spans[4].spanID]);
       manager._scrollToVisibleSpan(1);
       expect(scrollPastMock).lastCalledWith(4, 1);
       manager._scrollToVisibleSpan(-1);
       expect(scrollPastMock).lastCalledWith(4, -1);
     });
 
-    describe('scrollToNextVisibleSpan() and scrollToPrevVisibleSpan()', () => {
-      function getRefs(spanID) {
-        return [{ refType: 'CHILD_OF', spanID }];
-      }
+    it('scrolls to boundary when scrolling away from closest spanID in findMatches', () => {
+      const closetFindMatchesSpanID = 4;
+      accessors.getTopRowIndexVisible.mockReturnValue(closetFindMatchesSpanID - 1);
+      accessors.getBottomRowIndexVisible.mockReturnValue(closetFindMatchesSpanID + 1);
+      accessors.getSearchedSpanIDs = () => new Set([trace.spans[closetFindMatchesSpanID].spanID]);
+
+      manager._scrollToVisibleSpan(1);
+      expect(scrollPastMock).lastCalledWith(trace.spans.length - 1, 1);
+
+      manager._scrollToVisibleSpan(-1);
+      expect(scrollPastMock).lastCalledWith(0, -1);
+    });
+
+    it('scrolls to last visible row when boundary is hidden', () => {
+      const parentOfLastRowWithHiddenChildrenIndex = trace.spans.length - 2;
+      accessors.getBottomRowIndexVisible.mockReturnValue(0);
+      accessors.getCollapsedChildren = () =>
+        new Set([trace.spans[parentOfLastRowWithHiddenChildrenIndex].spanID]);
+      accessors.getSearchedSpanIDs = () => new Set([trace.spans[0].spanID]);
+      trace.spans[trace.spans.length - 1].references = getRefs(
+        trace.spans[parentOfLastRowWithHiddenChildrenIndex].spanID
+      );
+
+      manager._scrollToVisibleSpan(1);
+      expect(scrollPastMock).lastCalledWith(parentOfLastRowWithHiddenChildrenIndex, 1);
+    });
 
+    describe('scrollToNextVisibleSpan() and scrollToPrevVisibleSpan()', () => {
       beforeEach(() => {
         // change spans so 0 and 4 are top-level and their children are collapsed
         const spans = trace.spans;
@@ -213,6 +238,17 @@ describe('ScrollManager', () => {
         expect(scrollPastMock).lastCalledWith(4, -1);
       });
     });
+
+    describe('scrollToFirstVisibleSpan', () => {
+      beforeEach(() => {
+        jest.spyOn(manager, '_scrollToVisibleSpan').mockImplementationOnce();
+      });
+
+      it('calls _scrollToVisibleSpan searching downwards from first span', () => {
+        manager.scrollToFirstVisibleSpan();
+        expect(manager._scrollToVisibleSpan).toHaveBeenCalledWith(1, 0);
+      });
+    });
   });
 
   describe('scrollPageDown() and scrollPageUp()', () => {

packages/jaeger-ui/src/components/TracePage/ScrollManager.tsx

@@ -120,7 +120,7 @@ export default class ScrollManager {
     this._scroller.scrollTo(y);
   }
 
-  _scrollToVisibleSpan(direction: 1 | -1) {
+  _scrollToVisibleSpan(direction: 1 | -1, startRow?: number) {
     const xrs = this._accessors;
     /* istanbul ignore next */
     if (!xrs) {
@@ -131,7 +131,14 @@ export default class ScrollManager {
     }
     const { duration, spans, startTime: traceStartTime } = this._trace;
     const isUp = direction < 0;
-    const boundaryRow = isUp ? xrs.getTopRowIndexVisible() : xrs.getBottomRowIndexVisible();
+    let boundaryRow: number;
+    if (startRow != null) {
+      boundaryRow = startRow;
+    } else if (isUp) {
+      boundaryRow = xrs.getTopRowIndexVisible();
+    } else {
+      boundaryRow = xrs.getBottomRowIndexVisible();
+    }
     const spanIndex = xrs.mapRowIndexToSpanIndex(boundaryRow);
     if ((spanIndex === 0 && isUp) || (spanIndex === spans.length - 1 && !isUp)) {
       return;
@@ -184,7 +191,7 @@ export default class ScrollManager {
 
       // If there are hidden children, scroll to the last visible span
       if (childrenAreHidden) {
-        let isFallbackHidden: boolean | TNil;
+        let isFallbackHidden: boolean;
         do {
           const { isHidden, parentIDs } = isSpanHidden(spans[nextSpanIndex], childrenAreHidden, spansMap);
           if (isHidden) {
@@ -255,6 +262,10 @@ export default class ScrollManager {
     this._scrollToVisibleSpan(-1);
   };
 
+  scrollToFirstVisibleSpan = () => {
+    this._scrollToVisibleSpan(1, 0);
+  };
+
   destroy() {
     this._trace = undefined;
     this._scroller = undefined as any;

packages/jaeger-ui/src/components/TracePage/TraceGraph/OpNode.css

@@ -57,6 +57,7 @@ limitations under the License.
 .OpNode--popover .OpNode--copyIcon,
 .OpNode:not(:hover) .OpNode--copyIcon {
   color: transparent;
+  pointer-events: none;
 }
 
 .OpNode--service {

packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageHeader.test.js

@@ -14,6 +14,7 @@
 
 import React from 'react';
 import { shallow, mount } from 'enzyme';
+import { Link } from 'react-router-dom';
 
 import AltViewOptions from './AltViewOptions';
 import KeyboardShortcutsHelp from './KeyboardShortcutsHelp';
@@ -110,6 +111,14 @@ describe('<TracePageHeader>', () => {
       expect(wrapper.find(AltViewOptions).length).toBe(0);
     });
 
+    it('renders the link to search', () => {
+      expect(wrapper.find(Link).length).toBe(0);
+
+      const toSearch = 'some-link';
+      wrapper.setProps({ toSearch });
+      expect(wrapper.find({ to: toSearch }).length).toBe(1);
+    });
+
     it('toggles the standalone link', () => {
       const linkToStandalone = 'some-link';
       const props = {

packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageHeader.tsx

@@ -40,6 +40,7 @@ import './TracePageHeader.css';
 type TracePageHeaderEmbedProps = {
   canCollapse: boolean;
   clearSearch: () => void;
+  focusUiFindMatches: () => void;
   hideMap: boolean;
   hideSummary: boolean;
   linkToStandalone: string;
@@ -95,6 +96,7 @@ export function TracePageHeaderFn(props: TracePageHeaderEmbedProps & { forwarded
   const {
     canCollapse,
     clearSearch,
+    focusUiFindMatches,
     forwardedRef,
     hideMap,
     hideSummary,
@@ -161,17 +163,17 @@ export function TracePageHeaderFn(props: TracePageHeaderEmbedProps & { forwarded
         ) : (
           title
         )}
-        {showShortcutsHelp && <KeyboardShortcutsHelp className="ub-mr2" />}
         <TracePageSearchBar
           clearSearch={clearSearch}
+          focusUiFindMatches={focusUiFindMatches}
           nextResult={nextResult}
           prevResult={prevResult}
           ref={forwardedRef}
           resultCount={resultCount}
           textFilter={textFilter}
           navigable={!traceGraphView}
         />
-
+        {showShortcutsHelp && <KeyboardShortcutsHelp className="ub-m2" />}
         {showViewOptions && (
           <AltViewOptions
             onTraceGraphViewClicked={onTraceGraphViewClicked}

packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageSearchBar.css

@@ -15,22 +15,31 @@ limitations under the License.
 */
 
 .TracePageSearchBar {
-  max-width: 20rem;
+  width: 40%;
 }
 
 .TracePageSearchBar--bar {
-  display: flex;
   max-width: 20rem;
+  transition: max-width 0.5s;
+}
+
+.TracePageSearchBar--bar:focus-within {
+  max-width: 100%;
 }
 
 .TracePageSearchBar--count {
   opacity: 0.6;
 }
 
 .TracePageSearchBar--btn {
+  border-left: none;
   transition: 0.2s;
 }
 
 .TracePageSearchBar--btn.is-disabled {
   opacity: 0.5;
 }
+
+.TracePageSearchBar--locateBtn {
+  padding: 1px 8px 4px;
+}