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.

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 links to make values in tags or log properties clickable #223

Merged
merged 18 commits into from
Aug 3, 2018
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,22 @@ import IoIosArrowRight from 'react-icons/lib/io/ios-arrow-right';

import * as markers from './AccordianKeyValues.markers';
import KeyValuesTable from './KeyValuesTable';
import type { KeyValuePair, Link } from '../../../../types';

import './AccordianKeyValues.css';

type AccordianKeyValuesProps = {
className?: ?string,
data: { key: string, value: any }[],
data: KeyValuePair[],
highContrast?: boolean,
isOpen: boolean,
label: string,
linksGetter: ?(KeyValuePair[], number) => Link[],
onToggle: () => void,
};

// export for tests
export function KeyValuesSummary(props: { data?: { key: string, value: any }[] }) {
export function KeyValuesSummary(props: { data?: KeyValuePair[] }) {
const { data } = props;
if (!Array.isArray(data) || !data.length) {
return null;
Expand All @@ -59,7 +61,7 @@ KeyValuesSummary.defaultProps = {
};

export default function AccordianKeyValues(props: AccordianKeyValuesProps) {
const { className, data, highContrast, isOpen, label, onToggle } = props;
const { className, data, highContrast, isOpen, label, linksGetter, onToggle } = props;
const isEmpty = !Array.isArray(data) || !data.length;
const iconCls = cx('u-align-icon', { 'AccordianKeyValues--emptyIcon': isEmpty });
return (
Expand All @@ -80,7 +82,7 @@ export default function AccordianKeyValues(props: AccordianKeyValuesProps) {
</strong>
{!isOpen && <KeyValuesSummary data={data} />}
</div>
{isOpen && <KeyValuesTable data={data} />}
{isOpen && <KeyValuesTable data={data} linksGetter={linksGetter} />}
</div>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,13 @@ import IoIosArrowRight from 'react-icons/lib/io/ios-arrow-right';

import AccordianKeyValues from './AccordianKeyValues';
import { formatDuration } from '../utils';
import type { Log } from '../../../../types';
import type { Log, KeyValuePair, Link } from '../../../../types';

import './AccordianLogs.css';

type AccordianLogsProps = {
isOpen: boolean,
linksGetter: ?(KeyValuePair[], number) => Link[],
logs: Log[],
onItemToggle: Log => void,
onToggle: () => void,
Expand All @@ -35,7 +36,7 @@ type AccordianLogsProps = {
};

export default function AccordianLogs(props: AccordianLogsProps) {
const { isOpen, logs, openedItems, onItemToggle, onToggle, timestamp } = props;
const { isOpen, linksGetter, logs, openedItems, onItemToggle, onToggle, timestamp } = props;

return (
<div className="AccordianLogs">
Expand All @@ -59,6 +60,7 @@ export default function AccordianLogs(props: AccordianLogsProps) {
// compact
highContrast
isOpen={openedItems.has(log)}
linksGetter={linksGetter}
data={log.fields || []}
label={`${formatDuration(log.timestamp - timestamp)}`}
onToggle={() => onItemToggle(log)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,13 @@ limitations under the License.
white-space: pre;
width: 125px;
}

.KeyValueTable--body > tr > td {
padding: 0.25rem 0.5rem;
vertical-align: top;
}

.KeyValueTable--linkIcon {
vertical-align: middle;
font-weight: bold;
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import React from 'react';
import * as React from 'react';
import jsonMarkup from 'json-markup';
import { Dropdown, Icon, Menu } from 'antd';
Copy link
Member

Choose a reason for hiding this comment

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

This file looks great! Awesome.

(Not sure how to add a comment to a file instead of a line...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your encouraging feedback!

import type { KeyValuePair, Link } from '../../../../types';

import './KeyValuesTable.css';

Expand All @@ -30,27 +32,70 @@ function parseIfJson(value) {
return value;
}

const LinkValue = (props: { href: string, title?: string, children: React.Node }) => (
<a href={props.href} title={props.title} target="_blank" rel="noopener noreferrer">
{props.children} <Icon className="KeyValueTable--linkIcon" type="export" />
</a>
);

const linkValueList = (links: Link[]) => (
<Menu>
{links.map(({ text, url }, index) => (
// `index` is necessary in the key because url can repeat
// eslint-disable-next-line react/no-array-index-key
<Menu.Item key={`${url}-${index}`}>
<LinkValue href={url}>{text}</LinkValue>
</Menu.Item>
))}
</Menu>
);

type KeyValuesTableProps = {
data: { key: string, value: any }[],
data: KeyValuePair[],
linksGetter: ?(KeyValuePair[], number) => Link[],
};

export default function KeyValuesTable(props: KeyValuesTableProps) {
const { data } = props;
const { data, linksGetter } = props;
return (
<div className="KeyValueTable u-simple-scrollbars">
<table className="u-width-100">
<tbody className="KeyValueTable--body">
{data.map((row, i) => {
const jsonTable = (
// eslint-disable-next-line react/no-danger
<div dangerouslySetInnerHTML={{ __html: jsonMarkup(parseIfJson(row.value)) }} />
);
const markup = {
__html: jsonMarkup(parseIfJson(row.value)),
};
// eslint-disable-next-line react/no-danger
const jsonTable = <div className="ub-inline-block" dangerouslySetInnerHTML={markup} />;
const links = linksGetter ? linksGetter(data, i) : null;
let valueMarkup;
if (links && links.length === 1) {
valueMarkup = (
<div>
<LinkValue href={links[0].url} title={links[0].text}>
{jsonTable}
</LinkValue>
</div>
);
} else if (links && links.length > 1) {
valueMarkup = (
<div>
<Dropdown overlay={linkValueList(links)} placement="bottomRight" trigger={['click']}>
<a>
{jsonTable} <Icon className="KeyValueTable--linkIcon" type="profile" />
</a>
</Dropdown>
</div>
);
} else {
valueMarkup = jsonTable;
}
return (
// `i` is necessary in the key because row.key can repeat
// eslint-disable-next-line react/no-array-index-key
<tr key={`${row.key}-${i}`}>
<td className="KeyValueTable--keyColumn">{row.key}</td>
<td>{jsonTable}</td>
<td>{valueMarkup}</td>
Copy link
Member

Choose a reason for hiding this comment

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

Seems like the code above is sufficiently intricate to warrant being broken down into sub-components at the top of the file, similar to CustomNavDropdown. Something like LinkValue and LinkValueList where LinkValueList would compose LinkValue?

// Individual link
<LinkValue
  href={href}
  title={title}
  content={jsonMarkup}
/>

// List of links
<LinkValueList
  links={links}
  content={jsonMarkup}
/>

What do you think?

Also, I think it would make sense for the link to always show the icon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have taken into account this suggestion and updated the PR.
Note that the Dropdown component in its overlay property seems to work differently if there is a wrapper component such as LinkValueList before the Menu component, so I don't call LinkValueList as a component (<LinkValueList ... />) but directly as a function ({linkValueList(...)}).

Copy link
Member

Choose a reason for hiding this comment

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

Looks great, thank you.

</tr>
);
})}
Expand All @@ -59,3 +104,5 @@ export default function KeyValuesTable(props: KeyValuesTableProps) {
</div>
);
}

KeyValuesTable.LinkValue = LinkValue;
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

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

import KeyValuesTable from './KeyValuesTable';

Expand All @@ -38,4 +39,59 @@ describe('<KeyValuesTable>', () => {
expect(tr.find('.KeyValueTable--keyColumn').text()).toMatch(data[i].key);
});
});

it('renders a single link correctly', () => {
wrapper.setProps({
linksGetter: (array, i) =>
array[i].key === 'span.kind'
? [
{
url: `http://example.com/?kind=${encodeURIComponent(array[i].value)}`,
text: `More info about ${array[i].value}`,
},
]
: [],
});

const anchor = wrapper.find(KeyValuesTable.LinkValue);
expect(anchor).toHaveLength(1);
expect(anchor.prop('href')).toBe('http://example.com/?kind=client');
expect(anchor.prop('title')).toBe('More info about client');
expect(
anchor
.closest('tr')
.find('td')
.first()
.text()
).toBe('span.kind');
});

it('renders multiple links correctly', () => {
wrapper.setProps({
linksGetter: (array, i) =>
array[i].key === 'span.kind'
? [
{ url: `http://example.com/1?kind=${encodeURIComponent(array[i].value)}`, text: 'Example 1' },
{ url: `http://example.com/2?kind=${encodeURIComponent(array[i].value)}`, text: 'Example 2' },
]
: [],
});
const dropdown = wrapper.find(Dropdown);
const menu = shallow(dropdown.prop('overlay'));
const anchors = menu.find(KeyValuesTable.LinkValue);
expect(anchors).toHaveLength(2);
const firstAnchor = anchors.first();
expect(firstAnchor.prop('href')).toBe('http://example.com/1?kind=client');
expect(firstAnchor.children().text()).toBe('Example 1');
const secondAnchor = anchors.last();
expect(secondAnchor.prop('href')).toBe('http://example.com/2?kind=client');
expect(secondAnchor.children().text()).toBe('Example 2');
expect(
dropdown
.closest('tr')
.find('td')
.first()
.text()
).toBe('span.kind');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,13 @@ import AccordianLogs from './AccordianLogs';
import DetailState from './DetailState';
import { formatDuration } from '../utils';
import LabeledList from '../../../common/LabeledList';
import type { Log, Span } from '../../../../types';
import type { Log, Span, KeyValuePair, Link } from '../../../../types';

import './index.css';

type SpanDetailProps = {
detailState: DetailState,
linksGetter: ?(KeyValuePair[], number) => Link[],
logItemToggle: (string, Log) => void,
logsToggle: string => void,
processToggle: string => void,
Expand All @@ -37,7 +38,16 @@ type SpanDetailProps = {
};

export default function SpanDetail(props: SpanDetailProps) {
const { detailState, logItemToggle, logsToggle, processToggle, span, tagsToggle, traceStartTime } = props;
const {
detailState,
linksGetter,
logItemToggle,
logsToggle,
processToggle,
span,
tagsToggle,
traceStartTime,
} = props;
const { isTagsOpen, isProcessOpen, logs: logsState } = detailState;
const { operationName, process, duration, relativeStartTime, spanID, logs, tags } = span;
const overviewItems = [
Expand Down Expand Up @@ -73,6 +83,7 @@ export default function SpanDetail(props: SpanDetailProps) {
<AccordianKeyValues
data={tags}
label="Tags"
linksGetter={linksGetter}
isOpen={isTagsOpen}
onToggle={() => tagsToggle(spanID)}
/>
Expand All @@ -81,6 +92,7 @@ export default function SpanDetail(props: SpanDetailProps) {
className="ub-mb1"
data={process.tags}
label="Process"
linksGetter={linksGetter}
isOpen={isProcessOpen}
onToggle={() => processToggle(spanID)}
/>
Expand All @@ -89,6 +101,7 @@ export default function SpanDetail(props: SpanDetailProps) {
{logs &&
logs.length > 0 && (
<AccordianLogs
linksGetter={linksGetter}
logs={logs}
isOpen={logsState.isOpen}
openedItems={logsState.openedItems}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import SpanDetail from './SpanDetail';
import DetailState from './SpanDetail/DetailState';
import SpanTreeOffset from './SpanTreeOffset';
import TimelineRow from './TimelineRow';
import type { Log, Span } from '../../../types';
import type { Log, Span, KeyValuePair, Link } from '../../../types';

import './SpanDetailRow.css';

Expand All @@ -30,10 +30,12 @@ type SpanDetailRowProps = {
detailState: DetailState,
onDetailToggled: string => void,
isFilteredOut: boolean,
linksGetter: ?(number, KeyValuePair[], number) => Link[],
logItemToggle: (string, Log) => void,
logsToggle: string => void,
processToggle: string => void,
span: Span,
spanIndex: number,
tagsToggle: string => void,
traceStartTime: number,
};
Expand All @@ -45,6 +47,11 @@ export default class SpanDetailRow extends React.PureComponent<SpanDetailRowProp
this.props.onDetailToggled(this.props.span.spanID);
};

_linksGetter = (items: KeyValuePair[], itemIndex: number) => {
const { linksGetter, spanIndex } = this.props;
return linksGetter ? linksGetter(spanIndex, items, itemIndex) : [];
};

render() {
const {
color,
Expand Down Expand Up @@ -76,6 +83,7 @@ export default class SpanDetailRow extends React.PureComponent<SpanDetailRowProp
<div className="detail-info-wrapper" style={{ borderTopColor: color }}>
<SpanDetail
detailState={detailState}
linksGetter={this._linksGetter}
logItemToggle={logItemToggle}
logsToggle={logsToggle}
processToggle={processToggle}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@ describe('<SpanDetailRow>', () => {
columnDivision: 0.5,
detailState: new DetailState(),
onDetailToggled: jest.fn(),
linksGetter: jest.fn(),
isFilteredOut: false,
logItemToggle: jest.fn(),
logsToggle: jest.fn(),
processToggle: jest.fn(),
span: { spanID, depth: 3 },
spanIndex: 4,
tagsToggle: jest.fn(),
traceStartTime: 1000,
};
Expand All @@ -40,6 +42,7 @@ describe('<SpanDetailRow>', () => {

beforeEach(() => {
props.onDetailToggled.mockReset();
props.linksGetter.mockReset();
props.logItemToggle.mockReset();
props.logsToggle.mockReset();
props.processToggle.mockReset();
Expand Down Expand Up @@ -72,6 +75,7 @@ describe('<SpanDetailRow>', () => {
const spanDetail = (
<SpanDetail
detailState={props.detailState}
linksGetter={wrapper.instance()._linksGetter}
logItemToggle={props.logItemToggle}
logsToggle={props.logsToggle}
processToggle={props.processToggle}
Expand All @@ -82,4 +86,16 @@ describe('<SpanDetailRow>', () => {
);
expect(wrapper.contains(spanDetail)).toBe(true);
});

it('adds spanIndex when calling linksGetter', () => {
const spanDetail = wrapper.find(SpanDetail);
const linksGetter = spanDetail.prop('linksGetter');
const tags = [{ key: 'myKey', value: 'myValue' }];
const linksGetterResponse = {};
props.linksGetter.mockReturnValueOnce(linksGetterResponse);
const result = linksGetter(tags, 0);
expect(result).toBe(linksGetterResponse);
expect(props.linksGetter).toHaveBeenCalledTimes(1);
expect(props.linksGetter).toHaveBeenCalledWith(props.spanIndex, tags, 0);
});
});
Loading