Make left column adjustable in trace detail #74

tiffon · 2017-09-14T09:54:55Z

Column width of span name is now adjustable, within the bounds of [15%, 85%]. It's not clear in the gif, below, but the reduced opacity highlight color is present when dragging the column in either direction.

…-in-trace-detail

coveralls · 2017-09-14T09:59:18Z

Coverage decreased (-2.8%) to 57.505% when pulling 41eaff4 on milestone1/issue-63-left-column-adjustable-in-trace-detail into d68662c on master.

saminzadeh · 2017-09-15T16:09:13Z

src/components/TracePage/TraceTimelineViewer/SpanBarRow.js

@@ -68,7 +69,7 @@ export default function SpanBarRow(props) {
        ${isFilteredOut ? 'is-filtered-out' : ''}
      `}
    >
-      <TimelineRow.Left className="span-name-column">
+      <TimelineRow.Cell className="span-name-column" width={columnDivision}>


Nice, this is better with width

saminzadeh · 2017-09-15T16:10:20Z

src/components/TracePage/TraceTimelineViewer/Ticks.js


 import './Ticks.css';

-export default function Ticks(props) {
-  const { labels, ticks } = props;
+type TicksProps = {


Seeing a mixture of flowTypes and propTypes in this review. Let's stick with one

The PropTypes are holdovers from before. I've moved all of TraceTimelineViewer/* to use flow.

saminzadeh · 2017-09-15T16:10:49Z

src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow.css

+  background: rgba(136, 0, 136, 0.05);
+}
+
+/* .TimelineColumnResizer--dragger.isDraggingRight,


saminzadeh · 2017-09-15T16:15:14Z

src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow.js

+  _onWindowMouseUp = function _onWindowMouseUp({ clientX }) {
+    window.removeEventListener('mousemove', this._onWindowMouseMove);
+    window.removeEventListener('mouseup', this._onWindowMouseUp);
+    if (document && document.body && document.body.style) {


You can simplify with lodash _.get(document, 'style.body')

Thanks. I'd like to use idx, but it's not worth ejecting...

saminzadeh · 2017-09-15T16:18:29Z

src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow.js

+    this.setState({ rootX, currentDragPosition: clientX - (rootX || 0) });
+  };
+
+  _onWindowMouseUp = function _onWindowMouseUp({ clientX }) {


We seem to be doing a lot of stuff with dragging and complex calculations inline react components that can be hard to follow in future development.

Is there a solid library we can use that abstracts this stuff out or an HOC pattern we can reuse here?

Went ahead and extracted the calculations out into their own function and simplified the way the dimensions of the highlight are calculated (via using left, right instead of width—simpler calculations).

If you think it needs more changes, can you elaborate?

coveralls · 2017-09-16T10:31:11Z

Coverage decreased (-2.5%) to 57.766% when pulling 00e83d3 on milestone1/issue-63-left-column-adjustable-in-trace-detail into d68662c on master.

yurishkuro · 2017-09-16T16:32:08Z

@tiffon Please use "good commit message" guidelines in CONTRIBUTING.md when merging. The easiest way to do that is to make the PR titles follow the same rule, i.e. instead of "Fix #63 - Left column adjustable in trace detail", say "Make left column adjustable in trace detail", and "Fix #63" in the PR description (it doesn't need to be in the commit title since it's meaningless purely within Git, only makes sense in the context of GitHub and the PR).

yurishkuro · 2017-09-16T16:33:12Z

The s/s looks great!

…-in-trace-detail

coveralls · 2017-09-17T14:00:39Z

Coverage decreased (-2.9%) to 56.65% when pulling ae4c69e on milestone1/issue-63-left-column-adjustable-in-trace-detail into 981b1bb on master.

tiffon · 2017-09-17T14:18:24Z

@yurishkuro Thanks for adjusting the title and description.

Great to hear the s/s looks good 👍

saminzadeh · 2017-09-18T14:56:25Z

src/components/TracePage/TraceTimelineViewer/Ticks.js


 import './Ticks.css';

-export default function Ticks(props) {
-  const { labels, ticks } = props;
+type TicksProps = {


coveralls · 2017-09-19T02:48:36Z

Coverage decreased (-2.9%) to 56.662% when pulling b83dedd on milestone1/issue-63-left-column-adjustable-in-trace-detail into 981b1bb on master.

* Fix jaegertracing#63 Left column adjustable in trace detail * Simplify span-name column drag code * Remove unused hint styling * Trace view Loading indicator, handle next trace * Flow for remaining TraceTimelineViewer components * Use lodash get to simplify property access Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>

tiffon added 2 commits September 14, 2017 01:42

Fix #63 Left column adjustable in trace detail

dc8f2d4

Merge branch 'master' into milestone1/issue-63-left-column-adjustable…

41eaff4

…-in-trace-detail

tiffon requested review from jcorbin, whistlinwilly, saminzadeh and a team September 14, 2017 09:54

tiffon assigned saminzadeh Sep 14, 2017

saminzadeh reviewed Sep 15, 2017

View reviewed changes

tiffon added 2 commits September 16, 2017 03:24

Simplify span-name column drag code

00e83d3

Remove unused hint styling

3cc8d8d

yurishkuro changed the title ~~Fix #63 - Left column adjustable in trace detail~~ Make left column adjustable in trace detail Sep 16, 2017

tiffon added 3 commits September 17, 2017 05:27

Trace view Loading indicator, handle next trace

b76d79e

Merge branch 'master' into milestone1/issue-63-left-column-adjustable…

65e8085

…-in-trace-detail

Flow for remaining TraceTimelineViewer components

ae4c69e

saminzadeh approved these changes Sep 18, 2017

View reviewed changes

Use lodash get to simplify property access

b83dedd

tiffon merged commit f77ea5b into master Sep 21, 2017

yurishkuro unassigned saminzadeh Oct 23, 2017

yurishkuro deleted the milestone1/issue-63-left-column-adjustable-in-trace-detail branch January 29, 2020 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make left column adjustable in trace detail #74

Make left column adjustable in trace detail #74

tiffon commented Sep 14, 2017

coveralls commented Sep 14, 2017

saminzadeh Sep 15, 2017

saminzadeh Sep 15, 2017

tiffon Sep 17, 2017

saminzadeh Sep 18, 2017

saminzadeh Sep 15, 2017

tiffon Sep 16, 2017

saminzadeh Sep 15, 2017

tiffon Sep 17, 2017

saminzadeh Sep 15, 2017

tiffon Sep 16, 2017

coveralls commented Sep 16, 2017

yurishkuro commented Sep 16, 2017

yurishkuro commented Sep 16, 2017

coveralls commented Sep 17, 2017

tiffon commented Sep 17, 2017

saminzadeh Sep 18, 2017

coveralls commented Sep 19, 2017

Make left column adjustable in trace detail #74

Make left column adjustable in trace detail #74

Conversation

tiffon commented Sep 14, 2017

coveralls commented Sep 14, 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

coveralls commented Sep 16, 2017

yurishkuro commented Sep 16, 2017

yurishkuro commented Sep 16, 2017

coveralls commented Sep 17, 2017

tiffon commented Sep 17, 2017

Choose a reason for hiding this comment

coveralls commented Sep 19, 2017