add uiFind icons, scroll to first match #367

everett980 · 2019-04-02T21:24:10Z

Which problem is this PR solving?

Short description of the changes

Moves the click handler to add a spanID to uiFind to a button next to the spanID
Adds a collapse and scroll to match button to TracePageSearchBar
Scrolls to first uiFind match when the TimelineView is mounted

Screenshots of visual changes:

Unfocused Find

Aside from the icon button changes, the uiFind input looks the same before focusing

Focused Find

Focusing the input results in it widening

Icon close up

New Icon is currently hovered to show distinction. Vertical borders have been removed.

Deep link icon

New deep link icon, with tooltip (and with safari's scrollbar rearing its head)

Focused button

Slight highlight indicates focus.

Signed-off-by: Everett Ross <[email protected]>

everett980

Very much WIP to discuss better approaches than the setTimeouts

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/index.tsx

everett980 · 2019-04-02T21:25:57Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.tsx

@@ -158,6 +159,10 @@ export class VirtualizedTraceViewImpl extends React.PureComponent<VirtualizedTra
    setTrace(trace, uiFind);
  }

+  componentDidMount() {
+    setTimeout(this.props.scrollToFirstVisibleSpan);


Without the setTimeout it scrolls to the bottom. I think the rows may not have height in the initial render.
@tiffon do you have any insight on why this would scroll too far without the setTimeout

Looks like the constructor calls setTrace(...). When visiting the URL, directly, the setTrace(...) applies the find matching.

When coming from the search page the setTimeout has no effect; the find is already applied to the redux state via the reducer that changed the state to be viewing the trace.

So, seems like the rows are rendered but they're not collapsed, yet, when visiting a trace directly from a URL. Probably it's scrolling to the position of the row, but then the non-matching rows are collapsed and the target scroll position ends up getting truncated with the result of being scrolled to the bottom.

everett980 · 2019-04-02T21:27:45Z

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

+      focusUiFind: () => {
+        if (trace.data) {
+          focusUiFind(trace.data, uiFind);
+          setTimeout(this._scrollManager.scrollToFirstVisibleSpan);


The scroll destination changes as a result of dispatching focusUiFind. This setTimeout achieves the desired result but definitely seems like the wrong way to achieve this.
@tiffon how would you recommend scrolling to the first match only after the rows have been expanded/collapsed and had their children shown/hidden?

That's a tough one, maybe via componentDidUpdate() with an instance property indicating whether or not the component should scroll.

Also, I recommend turning this into a method instead of creating a new function with every render.

Unfortunately an instance property wouldn't work because the component that updates as a result of focusUiFind is not TracePage/index.tsx. I got around this by adding a property to the detailStates Map and childrenHiddenIDs Set made by calculateHiddenIdsAndDetailStates in TracePage/duck.tsx and reading that property in componentDidUpdate in TraceTimelineViewer/VirtualizedTraceView.tsx
It's slightly unorthodox in principle, but not so bad in practice.
Here is the commit that solely addresses setTimeout
TODO: more tests and icons

tiffon

Looks great! Left a few comments.

Also, what do you think of this change to the aesthetics of the search bar? (The borders are removed and the icon is shifted to be the first button.)

And, what do you think of this icon for the collapse-and-scroll?

(Akin to the maps icon to center the map on your current location.)

Last, currently, the search bar elements have a max width which is getting kind of not long enough given how many buttons we're adding. What do you think of removing that and letting it flex-grow when it's in use? The trace-name will probably need to have flex: 0 0 auto, in that case.

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

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/index.tsx

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/duck.tsx

tiffon · 2019-04-04T05:48:15Z

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

+      focusUiFind: () => {
+        if (trace.data) {
+          focusUiFind(trace.data, uiFind);
+          setTimeout(this._scrollManager.scrollToFirstVisibleSpan);


That's a tough one, maybe via componentDidUpdate() with an instance property indicating whether or not the component should scroll.

Also, I recommend turning this into a method instead of creating a new function with every render.

tiffon · 2019-04-04T06:02:33Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.tsx

@@ -158,6 +159,10 @@ export class VirtualizedTraceViewImpl extends React.PureComponent<VirtualizedTra
    setTrace(trace, uiFind);
  }

+  componentDidMount() {
+    setTimeout(this.props.scrollToFirstVisibleSpan);


Looks like the constructor calls setTrace(...). When visiting the URL, directly, the setTrace(...) applies the find matching.

When coming from the search page the setTimeout has no effect; the find is already applied to the redux state via the reducer that changed the state to be viewing the trace.

So, seems like the rows are rendered but they're not collapsed, yet, when visiting a trace directly from a URL. Probably it's scrolling to the position of the row, but then the non-matching rows are collapsed and the target scroll position ends up getting truncated with the result of being scrolled to the bottom.

TODO: Fix & add tests Signed-off-by: Everett Ross <[email protected]>

tests, add TraceTimelineViewer/duck.tsx tests Signed-off-by: Everett Ross <[email protected]>

codecov · 2019-04-11T21:02:18Z

Codecov Report

Merging #367 into master will increase coverage by 0.53%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
+ Coverage   88.12%   88.65%   +0.53%     
==========================================
  Files         157      157              
  Lines        3494     3518      +24     
  Branches      798      802       +4     
==========================================
+ Hits         3079     3119      +40     
+ Misses        378      364      -14     
+ Partials       37       35       -2

Impacted Files	Coverage Δ
...components/TracePage/TraceTimelineViewer/index.tsx	`100% <ø> (ø)`	⬆️
...ents/TracePage/TracePageHeader/TracePageHeader.tsx	`100% <100%> (+4.16%)`	⬆️
...eger-ui/src/components/TracePage/ScrollManager.tsx	`100% <100%> (+8.42%)`	⬆️
.../components/TracePage/TraceTimelineViewer/duck.tsx	`98.34% <100%> (+0.13%)`	⬆️
...kages/jaeger-ui/src/components/common/CopyIcon.tsx	`100% <100%> (ø)`	⬆️
...ts/TracePage/TraceTimelineViewer/SpanDetailRow.tsx	`100% <100%> (ø)`	⬆️
...s/TracePage/TracePageHeader/TracePageSearchBar.tsx	`100% <100%> (+12.5%)`	⬆️
...ePage/TraceTimelineViewer/VirtualizedTraceView.tsx	`92.37% <100%> (+3.28%)`	⬆️
...kages/jaeger-ui/src/components/TracePage/index.tsx	`100% <100%> (+2.27%)`	⬆️
...TracePage/TraceTimelineViewer/SpanDetail/index.tsx	`100% <100%> (ø)`	⬆️
... and 4 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 fe6c244...cdbdd9d. Read the comment docs.

Signed-off-by: Everett Ross <[email protected]>

Except for the possibility of removing Tween.tsx I believe all feedback has been addressed.

tiffon

Great work. Left some comments, let me know what you think.

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.tsx

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/duck.tsx

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

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

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

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/index.tsx

Signed-off-by: Everett Ross <[email protected]>

All changes have been implemented

tiffon

Looks great! Some minor comments. LMK if you want to talk about any of them.

tiffon · 2019-04-28T07:15:53Z

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

+  width: 40%;
+}
+
+.TracePageSearchBar--InputGroup {


This should have an initial lowercase:

.TracePageSearchBar--inputGroup

Also, you can use the util class .ub-justify-end.

Nice, updated
Where is the list of .ub- classes?

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

tiffon · 2019-04-28T07:22:03Z

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

      {/* style inline because compact overwrites the display */}
-      <Input.Group compact style={{ display: 'flex' }}>
+      <Input.Group className="TracePageSearchBar--InputGroup" compact style={{ display: 'flex' }}>


Please avoid using style unless the style is dynamic and needs to be calculated.

that's pre-existing. I can take a stab at fixing it though.

The comment above the inline style seems to be accurate. I'm going to leave it as is.

tiffon · 2019-04-28T07:33:22Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/index.tsx

@@ -69,6 +68,7 @@ export default function SpanDetail(props: SpanDetailProps) {
      value: formatDuration(relativeStartTime),
    },
  ];
+  const deepLinkCopyText = `${window.location.origin}${window.location.pathname}?uiFind=${spanID}`;


Why not just using location.origin, sans window?

Also, it's a bit strange having the magic string 'uiFind' in several places. I think this makes three. We can consolidate in a different PR.

I guess I'm just used to seeing window.location instead of location on its own, but location on its own works fine and is shorter so I updated.

I agree about consolidating uiFind in its own PR. Should that just be when uiFind is in a string? If so, this seems to be the only instance based on a quick search (ag --ts '['"'"'"`].+uiFind'). But I think it would make sense to be a constant whenever it is interacting with the URL (i.e.: here and when interacting with queryString.parse / queryString.stringify)

Also, eslint dislikes location but doesn't mind window.location. What is your preference on using window.location vs // eslint-disable-line no-restricted-globals

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.tsx

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/duck.tsx

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

packages/jaeger-ui/src/components/common/CopyIcon.tsx

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

Signed-off-by: Everett Ross <[email protected]>

tiffon

One quick question about focusing the find results from going back to the trace timeline view. Seems like that might be jarring?

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.tsx

tiffon · 2019-05-02T21:30:39Z

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

@@ -280,6 +280,9 @@ export class TracePageImpl extends React.PureComponent<TProps, TState> {
    if (this.props.trace && this.props.trace.data) {
      this.traceDagEV = calculateTraceDagEV(this.props.trace.data);
    }
+    if (traceGraphView) {
+      this.focusUiFindMatches();
+    }


This seems kind of strange. So, it will focus the UI find matches when you go back to the timeline view?

I thought I had removed this... It doesn't work without more changes and as you said it may not be the desired functionality.
Will really remove and push.

Signed-off-by: Everett Ross <[email protected]>

…0-improve-uiFind-interactions add uiFind icons, scroll to first match Signed-off-by: vvvprabhakar <[email protected]>

WIP - add uiFind icons, scroll to first match

a0e296a

Signed-off-by: Everett Ross <[email protected]>

ghost assigned everett980 Apr 2, 2019

ghost added the review label Apr 2, 2019

everett980 commented Apr 2, 2019

View reviewed changes

tiffon previously requested changes Apr 4, 2019

View reviewed changes

everett980 added 2 commits April 11, 2019 14:57

Use componentDidUpdate instead of setTimeout

ebf9473

TODO: Fix & add tests Signed-off-by: Everett Ross <[email protected]>

Clarify focusUiFind function name, fix existing

37f9a20

tests, add TraceTimelineViewer/duck.tsx tests Signed-off-by: Everett Ross <[email protected]>

everett980 changed the title ~~WIP - add uiFind icons, scroll to first match~~ add uiFind icons, scroll to first match Apr 11, 2019

everett980 added 4 commits April 11, 2019 17:21

Add scrollToFirstVisibleSpan tests

bb66f09

Signed-off-by: Everett Ross <[email protected]>

Add scrollToFirstVisibleSpan tests

79f04af

Signed-off-by: Everett Ross <[email protected]>

Bolster test coverage for files in PR

79eb6df

Signed-off-by: Everett Ross <[email protected]>

Improve TracePageSearchBar styling, choose icons

8a268c2

Signed-off-by: Everett Ross <[email protected]>

everett980 requested a review from tiffon April 15, 2019 19:55

tiffon previously requested changes Apr 20, 2019

View reviewed changes

everett980 added 4 commits April 23, 2019 18:18

Move scroll indicator from set/map to standalone

5a146ef

Signed-off-by: Everett Ross <[email protected]>

Update CopyIcon to support spanID deep link

43a8ca5

Signed-off-by: Everett Ross <[email protected]>

Fix CopyIcon css, Swap in copy-to-clipboard

5801d3e

Signed-off-by: Everett Ross <[email protected]>

Improve VirtualizedTraceView shouldComponentUpdate

73e8691

Signed-off-by: Everett Ross <[email protected]>

Merge branch 'master' into issue-361-and-360-improve-uiFind-interactions

7651dbc

ghost assigned tiffon Apr 26, 2019

tiffon suggested changes Apr 28, 2019

View reviewed changes

Remove unnecessary function calls, improve names

9b9f5c0

Signed-off-by: Everett Ross <[email protected]>

tiffon approved these changes May 2, 2019

View reviewed changes

Remove nonfunctional focusUiFindMatches call

cdbdd9d

Signed-off-by: Everett Ross <[email protected]>

everett980 merged commit d18004f into jaegertracing:master May 3, 2019

ghost removed the review label May 3, 2019

tiffon mentioned this pull request May 13, 2019

[trace timeline] Add a "collapse to matches" button next to the Find input #361

Closed

tiffon mentioned this pull request May 13, 2019

Unable to copy & paste span ID #360

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add uiFind icons, scroll to first match #367

add uiFind icons, scroll to first match #367

everett980 commented Apr 2, 2019 •

edited

Loading

everett980 left a comment

everett980 Apr 2, 2019

tiffon Apr 4, 2019

everett980 Apr 2, 2019

tiffon Apr 4, 2019

everett980 Apr 11, 2019

tiffon left a comment

tiffon Apr 4, 2019

tiffon Apr 4, 2019

codecov bot commented Apr 11, 2019 •

edited

Loading

tiffon left a comment

tiffon left a comment

tiffon Apr 28, 2019

tiffon Apr 28, 2019

everett980 May 2, 2019

tiffon Apr 28, 2019

everett980 Apr 29, 2019

everett980 May 2, 2019

tiffon Apr 28, 2019

everett980 May 2, 2019 •

edited

Loading

everett980 May 2, 2019

tiffon left a comment

tiffon May 2, 2019

everett980 May 3, 2019

add uiFind icons, scroll to first match #367

add uiFind icons, scroll to first match #367

Conversation

everett980 commented Apr 2, 2019 • edited Loading

Which problem is this PR solving?

Short description of the changes

Screenshots of visual changes:

Unfocused Find

Focused Find

Icon close up

Deep link icon

Focused button

everett980 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

codecov bot commented Apr 11, 2019 • edited Loading

Codecov Report

tiffon left a 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

everett980 May 2, 2019 • edited Loading

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

everett980 commented Apr 2, 2019 •

edited

Loading

codecov bot commented Apr 11, 2019 •

edited

Loading

everett980 May 2, 2019 •

edited

Loading