Span Search - Add result count, navigation and clear buttons #234

davit-y · 2018-08-13T21:37:36Z

Which problem is this PR solving?

Short description of the changes

Add result count
Add up/down arrows (call current nextVisibleSpan method)
Add clear button

TODO:

Improve aesthetics of disabled state
Tests
Add shortcuts

black-adder · 2018-08-13T21:39:01Z

Screen shots please

black-adder · 2018-08-13T21:40:16Z

bonus points if you screen shot your IDE with the code instead

davit-y · 2018-08-13T21:50:11Z

Without text filter:

With text filter:

tiffon

@davit-y Great work! Definitely a huge improvement! 🎉

Regarding the aesthetics, right now the disabled state has quite a bit of visual weight. I would say it would be preferable to lighten it up, if possible?

I left a number of small comments in the code. What do you think?

tiffon · 2018-08-13T22:01:42Z

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

@@ -30,14 +29,18 @@ import { formatDatetime, formatDuration } from '../../utils/date';
 import prefixUrl from '../../utils/prefix-url';

 import './TracePageHeader.css';
+import TracePageSearchBar from './TracePageSearchBar';


Please move this import to fall after TracePageHeader.track.

Imports are sorted alphabetically with more dots under less dots and css files that are imported for the side effect go below the other imports. Also, external packages go above local resources, separated by a blank line, with react going above others.

import * as React from 'react'; import extA from 'a'; import extB from 'b'; import a from './a'; import z from './z'; import b from '../b'; import 'a.css';

oops, forgot to push my commit

tiffon · 2018-08-13T22:03:16Z

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

-            onChange={event => updateTextFilter(event.target.value)}
-            defaultValue={textFilter}
-            data-test={markers.IN_TRACE_SEARCH}
+        <div className="ub-flex-auto ub-mr2 TracePageHeader--search">


Please move this wrapping <div> into the TracePageSearchBar component.

tiffon · 2018-08-13T22:04:55Z

packages/jaeger-ui/src/components/TracePage/TracePageSearchBar.js

+  resultCount: number,
+};
+
+type TracePageSearchBarState = {


Why do you need state? this.props.textFilter should be all you need.

tiffon · 2018-08-13T22:05:29Z

packages/jaeger-ui/src/components/TracePage/TracePageSearchBar.js

+
+    return (
+      <Input.Group compact style={{ display: 'flex' }}>
+        <Input // ^ inline because compact overwrites the display


Please move the comment above the line it refers to.

tiffon · 2018-08-13T22:05:48Z

packages/jaeger-ui/src/components/TracePage/TracePageSearchBar.js

+  render() {
+    const { prevResult, nextResult, resultCount } = this.props;
+
+    const count = this.state.textFilter ? <span>{resultCount.toString()}</span> : null;


I believe the .toString() call here is unnecessary.

I'd say it's preferable for the suffix to have less contrast than the input text:

const count = ... ? <span className="u-tx-muted">{resultCount}</span> : null;

the .toString() makes sure that "0" shows up, otherwise it's interpreted as null

I had wrapped with a span to add formatting, but forgot. u-tx-muted didn't seem to work so I created a style that drops the opacity.

Ok, good to know. But, looks like 0 is working ok with the suffix?

https://codesandbox.io/s/lp1r4yqz8q

tiffon · 2018-08-13T22:13:44Z

packages/jaeger-ui/src/components/TracePage/TracePageSearchBar.js

+    };
+  }
+
+  updateTextFilter = (textFilter: string) => {


If you change this to take the event as the parameter you do not need to create a function as the handler on every render.

tiffon · 2018-08-13T22:19:02Z

packages/jaeger-ui/src/components/TracePage/TracePageSearchBar.js

+        />
+        <Button disabled={!this.state.textFilter} icon="up" onClick={prevResult} />
+        <Button disabled={!this.state.textFilter} icon="down" onClick={nextResult} />
+        <Button disabled={!this.state.textFilter} icon="close" onClick={() => this.updateTextFilter('')} />


I recommend creating a method clearFilter = () => { ... } instead of creating a function on every render.

tiffon · 2018-08-13T22:42:48Z

packages/jaeger-ui/src/components/TracePage/index.js

@@ -45,6 +45,7 @@ import type { Config } from '../../types/config';
 import prefixUrl from '../../utils/prefix-url';

 import './index.css';
+import getFilteredSpans from './TraceTimelineViewer/get-filtered-spans';


Doesn't look like this file needs to be in the ./TraceTimelineViewer/ folder anymore?

tiffon · 2018-08-13T22:44:20Z

packages/jaeger-ui/src/components/TracePage/index.js

@@ -278,6 +291,9 @@ export default class TracePage extends React.PureComponent<TracePageProps, Trace
            traceID={traceID}
            onSlimViewClicked={this.toggleSlimView}
            textFilter={textFilter}
+            prevResult={this.prevResult}
+            nextResult={this.nextResult}


I don't think the use of methods is needed for the scroll props, e.g.

prevResult={this._scrollManager.scrollToPrevVisibleSpan}

I initially did this to set up the next PR where the buttons move one result at a time, but that will probably just live in the scrollManager too so I'll get rid of the methods.

tiffon · 2018-08-13T22:46:26Z

packages/jaeger-ui/src/components/TracePage/index.js

    trackFilter(textFilter);
    this.setState({ textFilter });
+    const findMatchesIDs = getFilteredSpans(this.props.trace, textFilter.trim());


I recommend setting findMatchesIDs to null if !textFilter.trim() ...

davit-y · 2018-08-14T16:55:43Z

Old disabled state:

Lighter disabled state:

tiffon · 2018-08-14T18:35:40Z

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

+      nextResult: () => {},
+      resultCount: 0,
+    };
+    wrapper = shallow(<TracePageSearchBar {...searchBarProps} />);


Seems like this should be in TracePageSearchBar.test.js?

tiffon · 2018-08-14T18:37:01Z

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

+  max-width: 20rem;
+}
+
+.TracePageSearchBar--count {


Was the u-tx-muted CSS utility class too dark?

tiffon · 2018-08-14T18:40:38Z

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

+  transition: 0.5s;
+}
+
+.TracePageSearchBar--btn-hide {


For consistency, please change the selector to something like .TracePageSearchBar--btn.is-* where * describes the state, e.g. .DiffNode--labelCell.is-added.

tiffon · 2018-08-14T18:46:33Z

packages/jaeger-ui/src/components/TracePage/TracePageSearchBar.js

+  render() {
+    const { prevResult, nextResult, resultCount } = this.props;
+
+    const count = this.state.textFilter ? <span>{resultCount.toString()}</span> : null;


Ok, good to know. But, looks like 0 is working ok with the suffix?

https://codesandbox.io/s/lp1r4yqz8q

tiffon

Thanks for adjusting the contrast on the buttons!

Left a couple of comments. What do you think?

tiffon · 2018-08-14T19:02:34Z

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

+  max-width: 20rem;
+}
+
+.TracePageSearchBar--count {


tiffon · 2018-08-14T19:14:01Z

packages/jaeger-ui/src/components/TracePage/index.js

    trackFilter(textFilter);
    this.setState({ textFilter });
+    this.setState({ findMatchesIDs });


These setState calls should be grouped into one call: this.setState({ a, b });

tiffon · 2018-08-14T19:16:34Z

packages/jaeger-ui/src/components/TracePage/index.js

@@ -61,7 +62,8 @@ type TracePageProps = {
 type TracePageState = {
  headerHeight: ?number,
  slimView: boolean,
-  textFilter: ?string,
+  textFilter: string,
+  findMatchesIDs: ?Set<string>,


A general comment about this component, the filter needs to be updated when the traceID changes, similar to the time range:

if (!(trace instanceof Error) && (!prevTrace || prevTrace.traceID !== trace.traceID)) { this.updateViewRangeTime(0, 1); }

tiffon · 2018-08-14T19:18:14Z

packages/jaeger-ui/src/components/TracePage/index.js

@@ -278,6 +292,9 @@ export default class TracePage extends React.PureComponent<TracePageProps, Trace
            traceID={traceID}
            onSlimViewClicked={this.toggleSlimView}
            textFilter={textFilter}
+            prevResult={this._scrollManager.scrollToPrevVisibleSpan}
+            nextResult={this._scrollManager.scrollToNextVisibleSpan}
+            resultCount={this.state.findMatchesIDs ? this.state.findMatchesIDs.size : 0}


I recommend destructing findMatchesIDs, similar to the other state value:

const { slimView, headerHeight, textFilter, viewRange } = this.state;

Signed-off-by: Davit Yeghshatyan <[email protected]>

tiffon

Great progress. A couple of small notes.

Also, there are 17 failing tests. Please take a look:

https://travis-ci.org/jaegertracing/jaeger-ui/builds/417057052#L2833

tiffon · 2018-08-17T00:50:09Z

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

@@ -86,7 +90,7 @@ export const HEADER_ITEMS = [
  },
 ];

-export default function TracePageHeader(props: TracePageHeaderProps) {
+function TracePageHeader(props: TracePageHeaderProps, ref: any) {


I recommend using the following type instead of any:

import { Input } from 'antd'; // ... ref: { current: Input | null }

tiffon · 2018-08-17T00:55:53Z

packages/jaeger-ui/src/components/TracePage/TracePageSearchBar.js

+  const { prevResult, nextResult, clearSearch, resultCount, updateTextFilter, textFilter } = props;
+
+  const count = textFilter ? (
+    <span className="TracePageSearchBar--count">{resultCount.toString()}</span>


Did you get a chance to try this without the .toString()? React should render the zero.

tiffon · 2018-08-17T00:56:53Z

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

+  });
+
+  it('calls updateTextFilter() function for onChange of the input', () => {
+    const updateTextFilter = sinon.spy();


Please use jest.fn() instead of sinon.spy().

tiffon · 2018-08-17T01:03:48Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/index.js

@@ -36,8 +36,8 @@ type TraceTimelineViewerProps = {
  collapseOne: (Span[]) => void,
  expandAll: () => void,
  expandOne: (Span[]) => void,
+  findMatchesIDs: Set<string>,


If this value can be null, then the type should be Set<string> | null

tiffon · 2018-08-17T01:06:09Z

packages/jaeger-ui/src/components/TracePage/index.js

@@ -99,6 +101,7 @@ export class TracePageImpl extends React.PureComponent<TracePageProps, TracePage
  state: TracePageState;

  _headerElm: ?Element;
+  _searchBar: any;


I recommend using the following type instead of any:

import { Input } from 'antd'; // ... ref: { current: Input | null }

tiffon · 2018-08-17T01:13:03Z

packages/jaeger-ui/src/components/TracePage/keyboard-shortcuts.js

@@ -64,6 +67,8 @@ export const kbdMappings = {
  expandAll: '[',
  collapseOne: 'p',
  expandOne: 'o',
+  searchSpans: 'ctrl+b',


For this, why not use meta ?

The keyboard handler adds the inputted character to the input. Meta+b outputs a character, while ctrl+b does not.

A work around to use meta is to remove all instances of the '∫' char from the input string, but I didn't want to clutter the code. I tried having the textFilter shortened by 1 char after the event goes off, but unfortunately the input box receives the char after the completion of the event.

Signed-off-by: Davit Yeghshatyan <[email protected]>

codecov · 2018-08-24T18:52:03Z

Codecov Report

Merging #234 into master will decrease coverage by 1%.
The diff coverage is 48%.

@@            Coverage Diff            @@
##           master    #234      +/-   ##
=========================================
- Coverage   78.61%   77.6%   -1.01%     
=========================================
  Files         135     135              
  Lines        2927    2934       +7     
  Branches      607     608       +1     
=========================================
- Hits         2301    2277      -24     
- Misses        495     520      +25     
- Partials      131     137       +6

Impacted Files	Coverage Δ
...components/TracePage/TracePageSearchBar.markers.js	`100% <ø> (ø)`
.../components/TracePage/TraceTimelineViewer/index.js	`100% <ø> (ø)`	⬆️
...c/components/TracePage/TraceTimelineViewer/duck.js	`100% <ø> (ø)`	⬆️
...-ui/src/components/TracePage/keyboard-shortcuts.js	`81.81% <ø> (ø)`	⬆️
.../src/components/TracePage/KeyboardShortcutsHelp.js	`20% <ø> (-13.34%)`	⬇️
...ger-ui/src/components/TracePage/TracePageHeader.js	`70% <0%> (-10%)`	⬇️
...cePage/TraceTimelineViewer/VirtualizedTraceView.js	`93.51% <100%> (-0.35%)`	⬇️
...ckages/jaeger-ui/src/components/TracePage/index.js	`76.1% <28.57%> (-18.95%)`	⬇️
...-ui/src/components/TracePage/TracePageSearchBar.js	`75% <75%> (ø)`
...ges/jaeger-ui/src/components/common/LabeledList.js	`20% <0%> (-80%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d148dc5...e23f6a1. Read the comment docs.

Signed-off-by: Davit Yeghshatyan <[email protected]>

tiffon · 2018-08-24T22:18:39Z

Looks great. Thanks for the hard work!

tiffon · 2018-09-25T12:29:16Z

@yurishkuro, can you merge this PR? I'll take care of the test coverage when I address test coverage in trace comparison.

…racing#234) * Add result count, navigation and clear buttons Signed-off-by: Davit Yeghshatyan <[email protected]> * Correct imports Signed-off-by: Davit Yeghshatyan <[email protected]> * Move and delete files Signed-off-by: Davit Yeghshatyan <[email protected]> * Review fixes Signed-off-by: Davit Yeghshatyan <[email protected]> * Fix TracePageHeader test Signed-off-by: Davit Yeghshatyan <[email protected]> * Lighten buttons when disabled Signed-off-by: Davit Yeghshatyan <[email protected]> * Review fixes Signed-off-by: Davit Yeghshatyan <[email protected]> * Fix tests Signed-off-by: Davit Yeghshatyan <[email protected]> * Add shortcuts Signed-off-by: Davit Yeghshatyan <[email protected]> * Fix merge changes Signed-off-by: Davit Yeghshatyan <[email protected]> * Fix TracePageHeader and TracePageSearchBar tests Signed-off-by: Davit Yeghshatyan <[email protected]> * Delay TracePageHeader testing until release of Enzyme v3.5.0 Signed-off-by: Davit Yeghshatyan <[email protected]> Signed-off-by: vvvprabhakar <[email protected]>

davit-y mentioned this pull request Aug 13, 2018

Improved Span Search #235

Closed

3 tasks

davit-y changed the title ~~Add result count, navigation and clear buttons~~ Span Search - Add result count, navigation and clear buttons Aug 13, 2018

tiffon suggested changes Aug 13, 2018

View reviewed changes

davit-y changed the title ~~Span Search - Add result count, navigation and clear buttons~~ [WIP] Span Search - Add result count, navigation and clear buttons Aug 14, 2018

tiffon reviewed Aug 14, 2018

View reviewed changes

tiffon suggested changes Aug 14, 2018

View reviewed changes

Davit Yeghshatyan added 9 commits August 16, 2018 19:30

Add result count, navigation and clear buttons

755b1d4

Signed-off-by: Davit Yeghshatyan <[email protected]>

Correct imports

df6f51b

Signed-off-by: Davit Yeghshatyan <[email protected]>

Move and delete files

407f1f7

Signed-off-by: Davit Yeghshatyan <[email protected]>

Review fixes

af12460

Signed-off-by: Davit Yeghshatyan <[email protected]>

Fix TracePageHeader test

5168c6e

Signed-off-by: Davit Yeghshatyan <[email protected]>

Lighten buttons when disabled

c46d954

Signed-off-by: Davit Yeghshatyan <[email protected]>

Review fixes

02dc925

Signed-off-by: Davit Yeghshatyan <[email protected]>

Fix tests

fe9df64

Signed-off-by: Davit Yeghshatyan <[email protected]>

Add shortcuts

cc48630

Signed-off-by: Davit Yeghshatyan <[email protected]>

davit-y force-pushed the search-ui-features branch from 70c2f60 to cc48630 Compare August 16, 2018 23:35

davit-y changed the title ~~[WIP] Span Search - Add result count, navigation and clear buttons~~ Span Search - Add result count, navigation and clear buttons Aug 16, 2018

Fix merge changes

7bdc815

Signed-off-by: Davit Yeghshatyan <[email protected]>

tiffon suggested changes Aug 17, 2018

View reviewed changes

Fix TracePageHeader and TracePageSearchBar tests

98ca290

Signed-off-by: Davit Yeghshatyan <[email protected]>

davit-y force-pushed the search-ui-features branch from cbba4ee to 98ca290 Compare August 22, 2018 15:24

This was referenced Aug 23, 2018

Span Search - Improve search logic #237

Merged

Span Search - Highlight search results #238

Merged

Delay TracePageHeader testing until release of Enzyme v3.5.0

e23f6a1

Signed-off-by: Davit Yeghshatyan <[email protected]>

davit-y force-pushed the search-ui-features branch from 534ab63 to e23f6a1 Compare August 24, 2018 21:44

tiffon approved these changes Aug 24, 2018

View reviewed changes

yurishkuro merged commit 3c6b248 into jaegertracing:master Sep 26, 2018

snyk-bot mentioned this pull request Jul 6, 2021

[Snyk] Security upgrade d3-zoom from 1.8.3 to 3.0.0 m3ntalsp00n/jaeger-ui#7

Open

Span Search - Add result count, navigation and clear buttons #234

Span Search - Add result count, navigation and clear buttons #234

Conversation

davit-y commented Aug 13, 2018 • edited Loading

Which problem is this PR solving?

Short description of the changes

TODO:

black-adder commented Aug 13, 2018

black-adder commented Aug 13, 2018

davit-y commented Aug 13, 2018

tiffon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davit-y commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiffon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiffon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 24, 2018 • edited Loading

Codecov Report

tiffon commented Aug 24, 2018

tiffon commented Sep 25, 2018

davit-y commented Aug 13, 2018 •

edited

Loading

codecov bot commented Aug 24, 2018 •

edited

Loading