feedback added

Signed-off-by: Philip Dengler <philip.dengler@novatec-gmbh.de>
jaegertracing · Apr 21, 2020 · 72aa762 · 72aa762
1 parent 3b2c011
commit 72aa762
Show file tree

Hide file tree

Showing 33 changed files with 267 additions and 270 deletions.
diff --git a/packages/jaeger-ui/src/components/TracePage/TracePageHeader/AltViewOptions.css b/packages/jaeger-ui/src/components/TracePage/TracePageHeader/AltViewOptions.css
@@ -0,0 +1,18 @@
+/*
+Copyright (c) 2020 The Jaeger Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+.span--AltViewOptions {
+  padding-right: 2rem;
+}
diff --git a/packages/jaeger-ui/src/components/TracePage/TracePageHeader/AltViewOptions.test.js b/packages/jaeger-ui/src/components/TracePage/TracePageHeader/AltViewOptions.test.js
@@ -81,31 +81,38 @@ describe('AltViewOptions', () => {
   });
 
   it('track dropdown menu', () => {
-    expect(trackGraphView).not.toHaveBeenCalled();
-    expect(props.onTraceViewChange).not.toHaveBeenCalled();
-    getLink('Trace Graph').simulate('click');
-    expect(trackGraphView).toHaveBeenCalledTimes(1);
-    expect(props.onTraceViewChange).toHaveBeenCalledTimes(1);
-    expect(trackStatisticsView).not.toHaveBeenCalled();
-    getLink('Trace Statistics').simulate('click');
-    expect(trackStatisticsView).toHaveBeenCalledTimes(1);
-    expect(props.onTraceViewChange).toHaveBeenCalledTimes(2);
+    const viewInteractions = [
+      {
+        link: 'Trace Graph',
+        trackFn: trackGraphView,
+        onTraceViewChangeArg: ETraceViewType.TraceGraph,
+      },
+      {
+        link: 'Trace Statistics',
+        trackFn: trackStatisticsView,
+        onTraceViewChangeArg: ETraceViewType.TraceStatisticsView,
+        propViewType: ETraceViewType.TraceGraph,
+      },
+      {
+        link: 'Trace Timeline',
+        trackFn: trackGanttView,
+        onTraceViewChangeArg: ETraceViewType.TraceTimelineViewer,
+        propViewType: ETraceViewType.TraceStatisticsView,
+      },
+    ];
 
-    wrapper.setProps({ viewType: ETraceViewType.TraceGraph });
-    expect(trackGanttView).not.toHaveBeenCalled();
-    getLink('Trace Timeline').simulate('click');
-    expect(trackGanttView).toHaveBeenCalledTimes(1);
-    expect(props.onTraceViewChange).toHaveBeenCalledTimes(3);
-    getLink('Trace Statistics').simulate('click');
-    expect(trackStatisticsView).toHaveBeenCalledTimes(2);
-    expect(props.onTraceViewChange).toHaveBeenCalledTimes(4);
+    viewInteractions.forEach(({ link, trackFn, propViewType }, i) => {
+      if (propViewType) {
+        wrapper.setProps({ viewType: propViewType });
+      }
+      expect(props.onTraceViewChange).toHaveBeenCalledTimes(i);
+      expect(trackFn).not.toHaveBeenCalled();
 
-    wrapper.setProps({ viewType: 2 });
-    getLink('Trace Timeline').simulate('click');
-    expect(trackGanttView).toHaveBeenCalledTimes(2);
-    expect(props.onTraceViewChange).toHaveBeenCalledTimes(5);
-    getLink('Trace Graph').simulate('click');
-    expect(trackGraphView).toHaveBeenCalledTimes(2);
-    expect(props.onTraceViewChange).toHaveBeenCalledTimes(6);
+      getLink(link).simulate('click');
+      expect(props.onTraceViewChange).toHaveBeenCalledTimes(i + 1);
+      viewInteractions.forEach(({ trackFn: fn }, j) => {
+        expect(fn).toHaveBeenCalledTimes(j <= i ? 1 : 0);
+      });
+    });
   });
 });
diff --git a/packages/jaeger-ui/src/components/TracePage/TracePageHeader/AltViewOptions.tsx b/packages/jaeger-ui/src/components/TracePage/TracePageHeader/AltViewOptions.tsx
@@ -13,8 +13,9 @@
 // limitations under the License.
 
 import * as React from 'react';
-import { Button, Dropdown, Icon, Menu } from 'antd';
+import { Dropdown, Icon, Menu } from 'antd';
 import { Link } from 'react-router-dom';
+import './AltViewOptions.css';
 
 import {
   trackGanttView,
@@ -32,7 +33,7 @@ type Props = {
   viewType: ETraceViewType;
 };
 
-const menuItems = [
+const MENU_ITEMS = [
   {
     viewType: ETraceViewType.TraceTimelineViewer,
     label: 'Trace Timeline',
@@ -50,7 +51,7 @@ const menuItems = [
 export default function AltViewOptions(props: Props) {
   const { onTraceViewChange, viewType, traceID } = props;
 
-  const handleToggleView = (item: ETraceViewType) => {
+  const handleSelectView = (item: ETraceViewType) => {
     if (item === ETraceViewType.TraceTimelineViewer) {
       trackGanttView();
     } else if (item === ETraceViewType.TraceGraph) {
@@ -63,15 +64,13 @@ export default function AltViewOptions(props: Props) {
 
   const menu = (
     <Menu>
-      {menuItems.map(item =>
-        item.viewType === viewType ? null : (
-          <Menu.Item key={item.label}>
-            <a onClick={() => handleToggleView(item.viewType)} role="button">
-              {item.label}
-            </a>
-          </Menu.Item>
-        )
-      )}
+      {MENU_ITEMS.filter(item => item.viewType !== viewType).map(item => (
+        <Menu.Item key={item.viewType}>
+          <a onClick={() => handleSelectView(item.viewType)} role="button">
+            {item.label}
+          </a>
+        </Menu.Item>
+      ))}
       <Menu.Item>
         <Link
           to={prefixUrl(`/api/traces/${traceID}?prettyPrint=true`)}
@@ -95,14 +94,14 @@ export default function AltViewOptions(props: Props) {
     </Menu>
   );
 
+  const currentItem = MENU_ITEMS.find(item => item.viewType === viewType);
+  const dropdownText = currentItem ? currentItem.label : 'Alternate Views';
   return (
     <Dropdown overlay={menu}>
-      <Button className="ub-mr2" htmlType="button">
-        {menuItems.find(test => test.viewType === viewType) !== undefined
-          ? menuItems.find(test => test.viewType === viewType)!.label
-          : ''}
+      <span className="span--AltViewOptions">
+        {`${dropdownText} `}
         <Icon type="down" />
-      </Button>
+      </span>
     </Dropdown>
   );
 }
diff --git a/packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageHeader.tsx b/packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageHeader.tsx
@@ -49,7 +49,7 @@ type TracePageHeaderEmbedProps = {
   nextResult: () => void;
   onArchiveClicked: () => void;
   onSlimViewClicked: () => void;
-  onTraceViewChange: (actualViewType: ETraceViewType) => void;
+  onTraceViewChange: (viewType: ETraceViewType) => void;
   prevResult: () => void;
   resultCount: number;
   showArchiveButton: boolean;

diff --git a/...ger-ui/src/components/TracePage/TracePageHeader/__snapshots__/AltViewOptions.test.js.snap b/...ger-ui/src/components/TracePage/TracePageHeader/__snapshots__/AltViewOptions.test.js.snap
@@ -54,18 +54,13 @@ exports[`AltViewOptions renders correctly 1`] = `
   placement="bottomLeft"
   prefixCls="ant-dropdown"
 >
-  <Button
-    block={false}
-    className="ub-mr2"
-    ghost={false}
-    htmlType="button"
-    loading={false}
-    prefixCls="ant-btn"
+  <span
+    className="span--AltViewOptions"
   >
-    Trace Timeline
+    Trace Timeline 
     <Icon
       type="down"
     />
-  </Button>
+  </span>
 </Dropdown>
 `;
diff --git a/packages/jaeger-ui/src/components/TracePage/TraceStatistics/DetailTableData.css b/packages/jaeger-ui/src/components/TracePage/TraceStatistics/DetailTableData.css
@@ -1,5 +1,5 @@
 /*
-Copyright (c) 2018 The Jaeger Authors.
+Copyright (c) 2020 The Jaeger Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -22,20 +22,6 @@ limitations under the License.
   color: black;
 }
 
-.DetailTableData--child--td {
-  border-left: 1px solid rgb(204, 204, 204);
-  border-right: 1px solid rgb(204, 204, 204);
-  border-top: 1px solid rgb(230, 230, 230);
-  border-bottom: 1px solid rgb(230, 230, 230);
-  width: 10%;
-  padding-left: 2em;
-
-  max-width: 25em;
-  overflow: hidden;
-  text-overflow: ellipsis;
-  white-space: nowrap;
-}
-
 .DetailTableData--serviceBorder {
   border-left: 4px solid transparent;
   padding-left: 0.6em;
@@ -46,9 +32,8 @@ limitations under the License.
   border-right: 1px solid rgb(204, 204, 204);
   border-top: 1px solid rgb(230, 230, 230);
   border-bottom: 1px solid rgb(230, 230, 230);
-  width: 9.09%;
-  padding-left: 2em;
   white-space: nowrap;
   overflow: hidden;
   text-overflow: ellipsis;
+  max-width: 25em;
 }
diff --git a/packages/jaeger-ui/src/components/TracePage/TraceStatistics/DetailTableData.test.js b/packages/jaeger-ui/src/components/TracePage/TraceStatistics/DetailTableData.test.js
@@ -1,4 +1,4 @@
-// Copyright (c) 2018 The Jaeger Authors.
+// Copyright (c) 2020 The Jaeger Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -31,15 +31,15 @@ describe('<DetailTableData>', () => {
         { title: 'Avg', attribute: 'avg', suffix: 'ms', isDecimal: true },
         { title: 'Min', attribute: 'min', suffix: 'ms', isDecimal: true },
         { title: 'Max', attribute: 'max', suffix: 'ms', isDecimal: true },
-        { title: 'Total ST', attribute: 'self', suffix: 'ms', isDecimal: true },
+        { title: 'ST Total', attribute: 'selfTotal', suffix: 'ms', isDecimal: true },
         { title: 'ST Avg', attribute: 'selfAvg', suffix: 'ms', isDecimal: true },
         { title: 'ST Min', attribute: 'selfMin', suffix: 'ms', isDecimal: true },
         { title: 'ST Max', attribute: 'selfMax', suffix: 'ms', isDecimal: true },
         { title: 'ST in Duration', attribute: 'percent', suffix: '%', isDecimal: true },
       ],
       name: 'GET /owners/5',
       searchColor: 'rgb(248,248,248)',
-      secondTagDropdownTitle: 'Operation Name',
+      valueNameSelector2: 'Operation Name',
       togglePopup: () => {},
       values: [3, 27.27, 9.09, 2.56, 13.73, 8.69, 2.9, 0.88, 5.06, 31.88],
     };
@@ -49,24 +49,24 @@ describe('<DetailTableData>', () => {
   it('does not explode', () => {
     expect(wrapper).toBeDefined();
     expect(wrapper.find('.DetailTableData--tr').length).toBe(1);
-    expect(wrapper.find('.DetailTableData--child--td').length).toBe(1);
-    expect(wrapper.find('.DetailTableData--td').length).toBe(10);
+    expect(wrapper.find('.DetailTableData--td').length).toBe(11);
   });
 
   it('renders TableOverviewHeadTag', () => {
     const firstRowColumns = wrapper.find('.DetailTableData--td').map(column => column.text());
-    expect(firstRowColumns.length).toBe(10);
+    expect(firstRowColumns.length).toBe(11);
 
-    expect(firstRowColumns[0]).toBe('3');
-    expect(firstRowColumns[1]).toBe('27.27ms');
-    expect(firstRowColumns[2]).toBe('9.09ms');
-    expect(firstRowColumns[3]).toBe('2.56ms');
-    expect(firstRowColumns[4]).toBe('13.73ms');
-    expect(firstRowColumns[5]).toBe('8.69ms');
-    expect(firstRowColumns[6]).toBe('2.90ms');
-    expect(firstRowColumns[7]).toBe('0.88ms');
-    expect(firstRowColumns[8]).toBe('5.06ms');
-    expect(firstRowColumns[9]).toBe('31.88%');
+    expect(firstRowColumns[0]).toBe('GET /owners/5');
+    expect(firstRowColumns[1]).toBe('3');
+    expect(firstRowColumns[2]).toBe('27.27ms');
+    expect(firstRowColumns[3]).toBe('9.09ms');
+    expect(firstRowColumns[4]).toBe('2.56ms');
+    expect(firstRowColumns[5]).toBe('13.73ms');
+    expect(firstRowColumns[6]).toBe('8.69ms');
+    expect(firstRowColumns[7]).toBe('2.90ms');
+    expect(firstRowColumns[8]).toBe('0.88ms');
+    expect(firstRowColumns[9]).toBe('5.06ms');
+    expect(firstRowColumns[10]).toBe('31.88%');
     expect(wrapper.find('.DetailTableData--tr').prop('style')).toEqual({
       background: 'rgb(248,248,248)',
       borderColor: 'rgb(248,248,248)',

diff --git a/packages/jaeger-ui/src/components/TracePage/TraceStatistics/DetailTableData.tsx b/packages/jaeger-ui/src/components/TracePage/TraceStatistics/DetailTableData.tsx
@@ -1,4 +1,4 @@
-// Copyright (c) 2018 The Jaeger Authors.
+// Copyright (c) 2020 The Jaeger Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -15,16 +15,17 @@
 import * as _ from 'lodash';
 import React, { Component } from 'react';
 import './DetailTableData.css';
+import { ITableValues, IColumnValue } from './types';
 
 type Props = {
   type: string;
   name: string;
   searchColor: string;
-  values: any[];
-  columnsArray: any[];
+  values: ITableValues[];
+  columnsArray: IColumnValue[];
   color: string;
   togglePopup: (name: string) => void;
-  secondTagDropdownTitle: string;
+  valueNameSelector2: string | null;
   colorToPercent: string;
 };
 
@@ -40,7 +41,6 @@ export default class DetailTableData extends Component<Props, State> {
     const element = this.props.values.map(item => {
       return { uid: _.uniqueId('id'), value: item };
     });
-
     this.setState(prevState => {
       return {
         ...prevState,
@@ -78,18 +78,18 @@ export default class DetailTableData extends Component<Props, State> {
     const labelStyle1 = { borderColor: this.props.color };
     const labelStyle2 = { borderColor: this.props.color, marginLeft: '12px' };
     let labelCondition;
-    if (this.props.secondTagDropdownTitle === 'Service Name') {
+    if (this.props.valueNameSelector2 === 'Service Name') {
       labelCondition = labelStyle2;
     } else {
       labelCondition = labelStyle1;
     }
     const onClickOption =
-      this.props.secondTagDropdownTitle === 'sql' && this.props.type !== others
+      this.props.valueNameSelector2 === 'sql' && this.props.type !== others
         ? () => this.props.togglePopup(this.props.name)
         : undefined;
     return (
       <tr className="DetailTableData--tr" style={styleCondition}>
-        <td className="DetailTableData--child--td">
+        <td className="DetailTableData--td">
           <a role="button" onClick={onClickOption} style={{ color: 'inherit' }}>
             <label title={this.props.name} className="DetailTableData--serviceBorder" style={labelCondition}>
               {this.props.name}

diff --git a/packages/jaeger-ui/src/components/TracePage/TraceStatistics/HeaderTable.css b/packages/jaeger-ui/src/components/TracePage/TraceStatistics/HeaderTable.css
@@ -1,5 +1,5 @@
 /*
-Copyright (c) 2018 The Jaeger Authors.
+Copyright (c) 2020 The Jaeger Authors.
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
 You may obtain a copy of the License at
@@ -19,14 +19,14 @@ limitations under the License.
 .HeaderTable--th {
   background: rgb(236, 236, 236);
   border: 1px solid rgb(204, 204, 204);
-  font-size: 100%;
-  font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif;
   color: rgb(35, 35, 35);
   font-weight: 15;
   line-height: 2em;
   padding-left: 1em;
 }
 
-.HeaderTable--buttonPosition {
-  float: right;
+.HeaderTable--sortButton {
+  display: flexbox;
+  flex-direction: row;
+  justify-content: space-around;
 }
diff --git a/packages/jaeger-ui/src/components/TracePage/TraceStatistics/HeaderTable.test.js b/packages/jaeger-ui/src/components/TracePage/TraceStatistics/HeaderTable.test.js
@@ -1,4 +1,4 @@
-// Copyright (c) 2018 The Jaeger Authors.
+// Copyright (c) 2020 The Jaeger Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.