Keyboard shortcuts and minimap UX #93

tiffon · 2017-10-14T01:53:18Z

Issues addressed:

In-trace search should allow navigation between results (partially addresses)
Fix [trace view] Drag and release on timeline header row zooms into respective range #89 - trace view - Drag and release on timeline header row zooms into respective range
Fix [trace view] Navigate and zoom via minimap #23 - trace view - Navigate and zoom via minimap
Fix [trace view] Pan and zoom via keyboard shortcuts #22 - trace view - Pan and zoom via keyboard shortcuts

Changes:

Revamped mouse UX on trace minimap
Adjusted rendering of the trace minimap to make spans more visible in large traces
Added visuals and mouse UX for zooming to trace timeline header row
Added keyboard shortcuts:
- Scroll up, down (w, s)
- Scroll to next, previous visible span (f, b)
- Pan left, right (a or left, d or right)
- Large pan left, right (shift+left, shift+right)
- Zoom in, out (up, down)
- Large zoom in, out (shift+up, shift+down)

TODO: fix tests. Primary reason for this refactor is to lay the ground-work for keyboard shortcuts. - Got rid of components/TracePage#context - components/TracePage#state.viewRange changed to have `.current` and `.next` with `.next` being the "unapplied" change from the mouse UX - Minimap UX now publishes unapplied changes (user dragging on minimap) to `viewRange.next` instead of retaining them in the minimap state - Minimap rendering changed to draw highlights based on `viewRange.next` regardless of UX state

- Color generator now also provides colors in RGB - ListView now has a public API to inspect the state of the ListView scrolling - VirtualizedTraceView now has a public API to inspect the state of the trace view - SpanGraph scrubbers have wider grips - SpanGraph minimap rendering update so the spans of large traces are more visible - Tween utility class added - scroll-page util added for animating window scrolling - ScrollManager added for scrolling to prev / next visible span, sensitive to search filtering - Keyboard shortcuts added for panning left, right, up down; zoom in, out; fast pan, zoom; skip to prev / next visible span TODO - Fix tests, add tests - Reorganize a bit to make things a bit smaller and more compartmentalized (a few things are getting a little large) - Refactor viewRange.next to support animating next position for both scrubbers

- Fixed a few tests, there are still broken tests - Fixed issue in minimap when page resized - Fixed issue in timeline header row mouse zoom when resize the columns

coveralls · 2017-10-14T01:59:17Z

Coverage decreased (-8.6%) to 47.997% when pulling 6aad7ed on issue-23-trace-minimap-navigate-zoom into 61cd543 on master.

coveralls · 2017-10-15T11:04:22Z

Coverage increased (+14.8%) to 71.399% when pulling 4ef1c55 on issue-23-trace-minimap-navigate-zoom into 61cd543 on master.

tiffon · 2017-10-16T21:46:44Z

src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.js

@@ -386,4 +446,4 @@ function mapDispatchToProps(dispatch) {
  return bindActionCreators(actions, dispatch);
 }

-export default connect(mapStateToProps, mapDispatchToProps)(VirtualizedTraceView);
+export default connect(mapStateToProps, mapDispatchToProps)(VirtualizedTraceViewImpl);


@saminzadeh You had a suggestion for a better name than *Impl (where Impl is short form of implementation)... What was that, again?

- Use options object for DraggableManager - Dont rerender CanvasSpanGraph unless trace changes - Remove rows, spans from ViewRange

coveralls · 2017-10-16T23:51:12Z

Coverage increased (+14.4%) to 71.004% when pulling 272cd64 on issue-23-trace-minimap-navigate-zoom into 61cd543 on master.

- Make trace minimap colors more prounounced

coveralls · 2017-10-17T00:36:27Z

Coverage increased (+14.4%) to 71.038% when pulling 3cc5817 on issue-23-trace-minimap-navigate-zoom into 61cd543 on master.

saminzadeh · 2017-10-19T17:03:10Z

package.json

@@ -83,7 +85,7 @@
    "clear-homepage": "json -I -f package.json -e 'delete this.homepage'",
    "deploy-docs": "./bin/deploy-docs.sh",
    "postpublish": "npm run build:docs && npm run deploy-docs",
-    "add-license": "uber-licence",
+    "add-license": "uber-licence && git co flow-typed",


Why git co flow-type?

saminzadeh · 2017-10-19T17:28:00Z

src/components/TracePage/ScrollManager.js

+   * `setAccessors` is bound in the ctor, so it can be passed as a prop to
+   * children components.
+   */
+  setAccessors = function setAccessors(accessors: Accessors) {


Can't you just do

{ setAccessors(accessors) { this.accessors = accessors } }

And then remove all those bindings in the constructor?

Investigate

saminzadeh · 2017-10-19T17:31:06Z

src/components/TracePage/ScrollManager.js

+ * ScrollManager is intended for scrolling the TracePage. Has two modes, paging
+ * and scrolling to the previous or next visible span.
+ */
+export default class ScrollManager {


I'm a bit impartial about the OOP vs functional style here. Seems like the codebase has a mix of it now? I can see why you went with OOP given the implementation, but do you think we should be consistent? More specifically, to reduce state and unintended side effects in modules.

saminzadeh · 2017-10-19T17:37:37Z

src/components/TracePage/SpanGraph/CanvasSpanGraph.test.js

+describe('<CanvasSpanGraph>', () => {
+  it('renders without exploding', () => {
+    const items = [{ valueWidth: 1, valueOffset: 1, serviceName: 'service-name-0' }];
+    const wrapper = mount(<CanvasSpanGraph items={[]} valueWidth={4000} />);


Can we do shallow rendering here instead? Mount requires more and will slow down tests from my understanding?
https://github.com/airbnb/enzyme/blob/master/docs/api/mount.md

Great call. mount(...) is necessary in a few spots, but this is not one of them 👍

saminzadeh · 2017-10-19T17:40:52Z

src/components/TracePage/TraceTimelineViewer/SpanBarRow.js

+              onClick={this._childrenToggle}
+            />
+            <a
+              className={`span-name ${isDetailExapnded ? 'is-detail-expanded' : ''}`}


Exapnded misspelled

saminzadeh · 2017-10-19T17:47:08Z

src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.js

  }

+  getViewRange = function getViewRange() {


I think this accessors are unnecessary abstraction. Just use the prop value directly, reduce the amount of boilerplate code.

Rename the props as necessary if you feel like it helps with readability.

saminzadeh

LGTM

coveralls · 2017-10-19T21:11:43Z

Coverage increased (+13.9%) to 70.516% when pulling 7a6fb5e on issue-23-trace-minimap-navigate-zoom into 61cd543 on master.

coveralls · 2017-10-19T21:33:41Z

Coverage increased (+13.9%) to 70.516% when pulling f41bcf6 on issue-23-trace-minimap-navigate-zoom into 61cd543 on master.

coveralls · 2017-10-19T21:55:24Z

Coverage increased (+13.7%) to 70.342% when pulling 2f22b02 on issue-23-trace-minimap-navigate-zoom into 61cd543 on master.

…navigate-zoom Keyboard shortcuts and minimap UX Signed-off-by: vvvprabhakar <[email protected]>

tiffon added 16 commits September 25, 2017 17:56

WIP - Drag to change view-range on trace minimap

b24825f

WIP - Drag to change view-range on trace minimap

f574cd0

Merge branch 'master' into issue-23-trace-minimap-navigate-zoom

d3b5a4f

Small adjustments to visuals of minimap UX

aef4225

Merge branch 'master' into issue-23-trace-minimap-navigate-zoom

82a790d

Merge branch 'master' into issue-23-trace-minimap-navigate-zoom

d27a809

Restructure components/TracePage#state.viewRange

c2745de

Perf improved when expanding row details (~60%)

d2cd518

Consolidate dragging into DraggableManager

7ab6fde

Column resize uses DraggableManager in trace view

2e33716

Misc cleanup

1e260f4

Dragging trace timeline time-labels zooms in

05efc77

Fix some tests, fix Draggable bug on resize window

45bd040

- Fixed a few tests, there are still broken tests - Fixed issue in minimap when page resized - Fixed issue in timeline header row mouse zoom when resize the columns

Test fixes, new tests for SpanGraph/ViewingLayer

6aad7ed

tiffon assigned saminzadeh Oct 14, 2017

tiffon requested review from jcorbin, whistlinwilly, saminzadeh and a team October 14, 2017 01:53

Add more tests

4ef1c55

tiffon mentioned this pull request Oct 16, 2017

In-trace search should allow navigation between results #91

Open

tiffon changed the title ~~[WIP] Keyboard shortcuts and minimap UX~~ Keyboard shortcuts and minimap UX Oct 16, 2017

tiffon commented Oct 16, 2017

View reviewed changes

Additional comments, misc tweaks

272cd64

- Use options object for DraggableManager - Dont rerender CanvasSpanGraph unless trace changes - Remove rows, spans from ViewRange

Fix trace minimap and timeline colors differ

3cc5817

- Make trace minimap colors more prounounced

saminzadeh reviewed Oct 19, 2017

View reviewed changes

Bind methods in the class body instead of ctor

7a6fb5e

saminzadeh approved these changes Oct 19, 2017

View reviewed changes

tiffon added 3 commits October 19, 2017 17:24

Better add-license adjustment

f41bcf6

Use shallow instead of mount

b2b8ae5

Fix "Exapnded" typo

2f22b02

tiffon merged commit b922904 into master Oct 20, 2017

yurishkuro unassigned saminzadeh Oct 23, 2017

yurishkuro deleted the issue-23-trace-minimap-navigate-zoom branch January 29, 2020 15:06

vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021

Merge pull request jaegertracing#93 from uber/issue-23-trace-minimap-…

1f0873c

…navigate-zoom Keyboard shortcuts and minimap UX Signed-off-by: vvvprabhakar <[email protected]>

snyk-bot mentioned this pull request Jan 18, 2022

[Snyk] Fix for 1 vulnerabilities m3ntalsp00n/jaeger-ui#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keyboard shortcuts and minimap UX #93

Keyboard shortcuts and minimap UX #93

tiffon commented Oct 14, 2017 •

edited

Loading

coveralls commented Oct 14, 2017

coveralls commented Oct 15, 2017

tiffon Oct 16, 2017

coveralls commented Oct 16, 2017

coveralls commented Oct 17, 2017

saminzadeh Oct 19, 2017

tiffon Oct 19, 2017

saminzadeh Oct 19, 2017

tiffon Oct 19, 2017

saminzadeh Oct 19, 2017

saminzadeh Oct 19, 2017

tiffon Oct 19, 2017

saminzadeh Oct 19, 2017

tiffon Oct 19, 2017

saminzadeh Oct 19, 2017

saminzadeh left a comment

coveralls commented Oct 19, 2017

coveralls commented Oct 19, 2017

coveralls commented Oct 19, 2017

Keyboard shortcuts and minimap UX #93

Keyboard shortcuts and minimap UX #93

Conversation

tiffon commented Oct 14, 2017 • edited Loading

coveralls commented Oct 14, 2017

coveralls commented Oct 15, 2017

Choose a reason for hiding this comment

coveralls commented Oct 16, 2017

coveralls commented Oct 17, 2017

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

saminzadeh left a comment

Choose a reason for hiding this comment

coveralls commented Oct 19, 2017

coveralls commented Oct 19, 2017

coveralls commented Oct 19, 2017

tiffon commented Oct 14, 2017 •

edited

Loading