Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add uiFind icons, scroll to first match #367

Merged
merged 14 commits into from
May 3, 2019
Merged

add uiFind icons, scroll to first match #367

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a0e296a
WIP - add uiFind icons, scroll to first match
everett980 Apr 2, 2019
ebf9473
Use componentDidUpdate instead of setTimeout
everett980 Apr 11, 2019
37f9a20
Clarify focusUiFind function name, fix existing
everett980 Apr 11, 2019
bb66f09
Add scrollToFirstVisibleSpan tests
everett980 Apr 11, 2019
79f04af
Add scrollToFirstVisibleSpan tests
everett980 Apr 11, 2019
79eb6df
Bolster test coverage for files in PR
everett980 Apr 12, 2019
8a268c2
Improve TracePageSearchBar styling, choose icons
everett980 Apr 15, 2019
5a146ef
Move scroll indicator from set/map to standalone
everett980 Apr 23, 2019
43a8ca5
Update CopyIcon to support spanID deep link
everett980 Apr 24, 2019
5801d3e
Fix CopyIcon css, Swap in copy-to-clipboard
everett980 Apr 25, 2019
73e8691
Improve VirtualizedTraceView shouldComponentUpdate
everett980 Apr 25, 2019
7651dbc
Merge branch 'master' into issue-361-and-360-improve-uiFind-interactions
tiffon Apr 26, 2019
9b9f5c0
Remove unnecessary function calls, improve names
everett980 May 2, 2019
cdbdd9d
Remove nonfunctional focusUiFindMatches call
everett980 May 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update CopyIcon to support spanID deep link 
Signed-off-by: Everett Ross <reverett@uber.com>
  • Loading branch information
@everett980
everett980 committed Apr 24, 2019
commit 43a8ca5ab36f5092961d28a18db7d422b6cc4998
24 changes: 24 additions & 0 deletions ...ges/jaeger-ui/src/components/TraceDiff/TraceDiffGraph/__snapshots__/drawNode.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ exports[`drawNode diffNode renders as expected when props.a and props.b are the
<CopyIcon
className="DiffNode--copyIcon"
copyText="serviceName operationName"
icon="copy"
placement="left"
tooltipTitle="Copy label"
/>
</td>
Expand Down Expand Up @@ -70,6 +72,8 @@ exports[`drawNode diffNode renders as expected when props.a and props.b are the
<CopyIcon
className="DiffNode--copyIcon"
copyText="serviceName operationName"
icon="copy"
placement="left"
tooltipTitle="Copy label"
/>
</td>
Expand Down Expand Up @@ -116,6 +120,8 @@ exports[`drawNode diffNode renders as expected when props.a is 0 1`] = `
<CopyIcon
className="DiffNode--copyIcon"
copyText="serviceName operationName"
icon="copy"
placement="left"
tooltipTitle="Copy label"
/>
</td>
Expand Down Expand Up @@ -181,6 +187,8 @@ exports[`drawNode diffNode renders as expected when props.a is 0 1`] = `
<CopyIcon
className="DiffNode--copyIcon"
copyText="serviceName operationName"
icon="copy"
placement="left"
tooltipTitle="Copy label"
/>
</td>
Expand Down Expand Up @@ -242,6 +250,8 @@ exports[`drawNode diffNode renders as expected when props.a is less than props.b
<CopyIcon
className="DiffNode--copyIcon"
copyText="serviceName operationName"
icon="copy"
placement="left"
tooltipTitle="Copy label"
/>
</td>
Expand Down Expand Up @@ -307,6 +317,8 @@ exports[`drawNode diffNode renders as expected when props.a is less than props.b
<CopyIcon
className="DiffNode--copyIcon"
copyText="serviceName operationName"
icon="copy"
placement="left"
tooltipTitle="Copy label"
/>
</td>
Expand Down Expand Up @@ -368,6 +380,8 @@ exports[`drawNode diffNode renders as expected when props.a is more than props.b
<CopyIcon
className="DiffNode--copyIcon"
copyText="serviceName operationName"
icon="copy"
placement="left"
tooltipTitle="Copy label"
/>
</td>
Expand Down Expand Up @@ -433,6 +447,8 @@ exports[`drawNode diffNode renders as expected when props.a is more than props.b
<CopyIcon
className="DiffNode--copyIcon"
copyText="serviceName operationName"
icon="copy"
placement="left"
tooltipTitle="Copy label"
/>
</td>
Expand Down Expand Up @@ -494,6 +510,8 @@ exports[`drawNode diffNode renders as expected when props.b is 0 1`] = `
<CopyIcon
className="DiffNode--copyIcon"
copyText="serviceName operationName"
icon="copy"
placement="left"
tooltipTitle="Copy label"
/>
</td>
Expand Down Expand Up @@ -559,6 +577,8 @@ exports[`drawNode diffNode renders as expected when props.b is 0 1`] = `
<CopyIcon
className="DiffNode--copyIcon"
copyText="serviceName operationName"
icon="copy"
placement="left"
tooltipTitle="Copy label"
/>
</td>
Expand Down Expand Up @@ -615,6 +635,8 @@ exports[`drawNode diffNode renders as expected when props.isUiFindMatch is true
<CopyIcon
className="DiffNode--copyIcon"
copyText="serviceName operationName"
icon="copy"
placement="left"
tooltipTitle="Copy label"
/>
</td>
Expand Down Expand Up @@ -660,6 +682,8 @@ exports[`drawNode diffNode renders as expected when props.isUiFindMatch is true
<CopyIcon
className="DiffNode--copyIcon"
copyText="serviceName operationName"
icon="copy"
placement="left"
tooltipTitle="Copy label"
/>
</td>
Expand Down
15 changes: 8 additions & 7 deletions packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@
jest.mock('../utils');

import React from 'react';
import { Button } from 'antd';
import { shallow } from 'enzyme';

import AccordianKeyValues from './AccordianKeyValues';
import AccordianLogs from './AccordianLogs';
import DetailState from './DetailState';
import SpanDetail from './index';
import { formatDuration } from '../utils';
import CopyIcon from '../../../common/CopyIcon';
import LabeledList from '../../../common/LabeledList';
import traceGenerator from '../../../../demo/trace-generators';
import transformTraceData from '../../../../model/transform-trace-data';
Expand All @@ -39,7 +39,6 @@ describe('<SpanDetail>', () => {
.toggleTags();
const traceStartTime = 5;
const props = {
addToUiFind: jest.fn(),
detailState,
span,
traceStartTime,
Expand All @@ -65,7 +64,6 @@ describe('<SpanDetail>', () => {
props.processToggle.mockReset();
props.logsToggle.mockReset();
props.logItemToggle.mockReset();
props.addToUiFind.mockReset();
wrapper = shallow(<SpanDetail {...props} />);
});

Expand Down Expand Up @@ -122,9 +120,12 @@ describe('<SpanDetail>', () => {
expect(props.logItemToggle).toHaveBeenLastCalledWith(span.spanID, somethingUniq);
});

it('calls addToUiFind when the spanID is clicked', () => {
const spanIDButton = wrapper.find(Button);
spanIDButton.simulate('click');
expect(props.addToUiFind).toHaveBeenCalledWith(props.span.spanID);
it('renders CopyIcon with deep link URL', () => {
expect(
wrapper
.find(CopyIcon)
.prop('copyText')
.includes(`?uiFind=${props.span.spanID}`)
).toBe(true);
});
});
22 changes: 9 additions & 13 deletions packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@
// limitations under the License.

import React from 'react';
import { Button, Divider, Tooltip } from 'antd';
import ZoomIn from 'react-icons/lib/ti/zoom-in';
import { Divider } from 'antd';

import AccordianKeyValues from './AccordianKeyValues';
import AccordianLogs from './AccordianLogs';
import DetailState from './DetailState';
import { formatDuration } from '../utils';
import CopyIcon from '../../../common/CopyIcon';
import LabeledList from '../../../common/LabeledList';

import { TNil } from '../../../../types';
Expand All @@ -28,7 +28,6 @@ import { Log, Span, KeyValuePair, Link } from '../../../../types/trace';
import './index.css';

type SpanDetailProps = {
addToUiFind: (spanID: string) => void;
detailState: DetailState;
linksGetter: ((links: KeyValuePair[], index: number) => Link[]) | TNil;
logItemToggle: (spanID: string, log: Log) => void;
Expand All @@ -41,7 +40,6 @@ type SpanDetailProps = {

export default function SpanDetail(props: SpanDetailProps) {
const {
addToUiFind,
detailState,
linksGetter,
logItemToggle,
Expand Down Expand Up @@ -70,6 +68,7 @@ export default function SpanDetail(props: SpanDetailProps) {
value: formatDuration(relativeStartTime),
},
];
const deepLinkCopyText = `${window.location.origin}${window.location.pathname}?uiFind=${spanID}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@everett980 everett980 May 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return (
<div>
Expand Down Expand Up @@ -116,15 +115,12 @@ export default function SpanDetail(props: SpanDetailProps) {
)}
<small className="SpanDetail--debugInfo">
<span className="SpanDetail--debugLabel" data-label="SpanID:" /> {spanID}
<Tooltip placement="topRight" title="Click to add to filter for deep linking">
<Button
className="SpanDetail--debugValue ant-btn-icon-only"
htmlType="button"
onClick={() => addToUiFind(spanID)}
>
<ZoomIn size="1.4em" />
</Button>
</Tooltip>
<CopyIcon
copyText={deepLinkCopyText}
icon="link"
placement="top"
tooltipTitle="Copy deep link to this span"
/>
</small>
</div>
</div>
Expand Down
3 changes: 0 additions & 3 deletions packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetailRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import { Log, Span, KeyValuePair, Link } from '../../../types/trace';
import './SpanDetailRow.css';

type SpanDetailRowProps = {
addToUiFind: (spanID: string) => void;
color: string;
columnDivision: number;
detailState: DetailState;
Expand All @@ -51,7 +50,6 @@ export default class SpanDetailRow extends React.PureComponent<SpanDetailRowProp

render() {
const {
addToUiFind,
color,
columnDivision,
detailState,
Expand Down Expand Up @@ -79,7 +77,6 @@ export default class SpanDetailRow extends React.PureComponent<SpanDetailRowProp
<TimelineRow.Cell width={1 - columnDivision}>
<div className="detail-info-wrapper" style={{ borderTopColor: color }}>
<SpanDetail
addToUiFind={addToUiFind}
detailState={detailState}
linksGetter={this._linksGetter}
logItemToggle={logItemToggle}
Expand Down
13 changes: 0 additions & 13 deletions packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import { Accessors } from '../ScrollManager';
import { extractUiFindFromState, TExtractUiFindFromStateReturn } from '../../common/UiFindInput';
import getLinks from '../../../model/link-patterns';
import colorGenerator from '../../../utils/color-generator';
import updateUiFind from '../../../utils/update-ui-find';
import { TNil, ReduxState } from '../../../types';
import { Log, Span, Trace, KeyValuePair } from '../../../types/trace';
import TTraceTimeline from '../../../types/TTraceTimeline';
Expand Down Expand Up @@ -224,17 +223,6 @@ export class VirtualizedTraceViewImpl extends React.PureComponent<VirtualizedTra
}
}

addToUiFind = (addition: string) => {
const { uiFind, history, location } = this.props;
if (!uiFind || !uiFind.includes(addition)) {
updateUiFind({
history,
location,
uiFind: cx(uiFind, addition),
});
}
};

getAccessors() {
const lv = this.listView;
if (!lv) {
Expand Down Expand Up @@ -401,7 +389,6 @@ export class VirtualizedTraceViewImpl extends React.PureComponent<VirtualizedTra
return (
<div className="VirtualizedTraceView--row" key={key} style={{ ...style, zIndex: 1 }} {...attrs}>
<SpanDetailRow
addToUiFind={this.addToUiFind}
color={color}
columnDivision={spanNameColumnWidth}
onDetailToggled={detailToggle}
Expand Down
6 changes: 3 additions & 3 deletions packages/jaeger-ui/src/components/common/CopyIcon.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

import React from 'react';
import { shallow } from 'enzyme';
import { Icon, Tooltip } from 'antd';
import { Button, Tooltip } from 'antd';

import CopyIcon from './CopyIcon';

Expand All @@ -34,9 +34,9 @@ describe('<CopyIcon />', () => {
expect(wrapper).toMatchSnapshot();
});

it('updates state when icon is clicked', () => {
it('updates state when clicked', () => {
expect(wrapper.state().hasCopied).toBe(false);
wrapper.find(Icon).simulate('click');
wrapper.find(Button).simulate('click');
expect(wrapper.state().hasCopied).toBe(true);
});

Expand Down
17 changes: 14 additions & 3 deletions packages/jaeger-ui/src/components/common/CopyIcon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,16 @@

import * as React from 'react';

import { Icon, Tooltip } from 'antd';
import { Tooltip, Button } from 'antd';
import { TooltipPlacement } from 'antd/lib/tooltip/index';

import CopyToClipboard from 'react-copy-to-clipboard';

type PropsType = {
className?: string;
copyText: string;
icon?: string;
placement?: TooltipPlacement;
everett980 marked this conversation as resolved.
Show resolved Hide resolved
tooltipTitle: string;
};

Expand All @@ -30,6 +34,8 @@ type StateType = {
export default class CopyIcon extends React.PureComponent<PropsType, StateType> {
static defaultProps: Partial<PropsType> = {
className: undefined,
icon: 'copy',
placement: 'left',
};

state = {
Expand All @@ -56,11 +62,16 @@ export default class CopyIcon extends React.PureComponent<PropsType, StateType>
arrowPointAtCenter
mouseLeaveDelay={0.5}
onVisibleChange={this.handleTooltipVisibilityChange}
placement="left"
placement={this.props.placement}
title={this.state.hasCopied ? 'Copied' : this.props.tooltipTitle}
>
<CopyToClipboard text={this.props.copyText}>
<Icon className={this.props.className} onClick={this.handleClick} type="copy" />
<Button
className={this.props.className}
htmlType="button"
onClick={this.handleClick}
icon={this.props.icon}
/>
</CopyToClipboard>
</Tooltip>
);
Expand Down
9 changes: 7 additions & 2 deletions packages/jaeger-ui/src/components/common/__snapshots__/CopyIcon.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,15 @@ exports[`<CopyIcon /> renders as expected 1`] = `
<CopyToClipboard
text="copyTextValue"
>
<Icon
<Button
block={false}
className="classNameValue"
ghost={false}
htmlType="button"
icon="copy"
loading={false}
onClick={[Function]}
type="copy"
prefixCls="ant-btn"
/>
</CopyToClipboard>
</Tooltip>
Expand Down