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

Refactors properties table, implements typescript #22402

Merged
merged 9 commits into from
Sep 3, 2018
9 changes: 1 addition & 8 deletions x-pack/plugins/apm/jsconfig.json
Original file line number Diff line number Diff line change
@@ -1,11 +1,4 @@
{
"compilerOptions": {
"target": "es6",
"module": "commonjs",
"baseUrl": "../../../.",
"paths": {
"ui/*": ["src/ui/public/*"]
}
},
"extends": "../../tsconfig.json",
"exclude": ["node_modules", "**/node_modules/*", "build"]
}
Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import _ from 'lodash';
import React from 'react';
import styled from 'styled-components';
import {
colors,
fontFamilyCode,
fontSizes,
px,
units
} from '../../../style/variables';

interface StringMap<T> {
[key: string]: T;
}

type KeySorter = (data: StringMap<any>, parentKey?: string) => string[];

const Table = styled.table`
font-family: ${fontFamilyCode};
font-size: ${fontSizes.small};
width: 100%;
`;

const Row = styled.tr`
border-bottom: ${px(1)} solid ${colors.gray4};
&:last-child {
border: 0;
}
`;

const Cell = styled.td`
vertical-align: top;
padding: ${px(units.half)} 0;

${Row}:first-child> & {
padding-top: 0;
}

${Row}:last-child> & {
padding-bottom: 0;
}

&:first-child {
width: ${px(units.unit * 20)};
font-weight: bold;
}
`;

const EmptyValue = styled.span`
color: ${colors.gray3};
`;

export function FormattedKey({
k,
value
}: {
k: string;
value: any;
}): JSX.Element {
if (value == null) {
return <EmptyValue>{k}</EmptyValue>;
}

return <React.Fragment>{k}</React.Fragment>;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a bit harder on the eyes compared to formatValue.

Copy link
Member

Choose a reason for hiding this comment

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

(Harder on the eyes, mostly due to the function signature, where the object param makes types more verbose)


export function FormattedValue({ value }: { value: any }): JSX.Element {
if (_.isObject(value)) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it just me, or is the lodash import missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right ... but how is the linter not picking this up? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I was wondering about that too! Linter is picking it up if other things are used but not defined - but not for lodash. Perhaps it's declared as global somewhere - not sure.

Does it throw errors in runtime, or does it just work?

return <pre>{JSON.stringify(value, null, 4)}</pre>;
} else if (_.isBoolean(value) || _.isNumber(value)) {
return <React.Fragment>{String(value)}</React.Fragment>;
} else if (!value) {
return <EmptyValue>N/A</EmptyValue>;
}

return <React.Fragment>{value}</React.Fragment>;
}
Copy link
Member

Choose a reason for hiding this comment

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

Again, I preferred the formatValue function. But I'm fine with this if you prefer that.


export function NestedValue({
parentKey,
value,
depth,
keySorter
}: {
value: any;
depth: number;
parentKey?: string;
keySorter?: KeySorter;
}): JSX.Element {
if (depth > 0 && _.isObject(value)) {
return (
<NestedKeyValueTable
data={value}
parentKey={parentKey}
keySorter={keySorter}
depth={depth - 1}
/>
);
}

return <FormattedValue value={value} />;
}

export function NestedKeyValueTable({
data,
parentKey,
keySorter = Object.keys,
depth = 0
}: {
data: StringMap<any>;
parentKey?: string;
keySorter?: KeySorter;
depth?: number;
}): JSX.Element {
return (
<Table>
<tbody>
{keySorter(data, parentKey).map((key: string) => (
Copy link
Member

Choose a reason for hiding this comment

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

Since keySorter is types and will return string[] is key: string redundant?

This is also a more general question: should we avoid adding types where it can be inferred, or eagerly add it to be safe?

Copy link
Member

Choose a reason for hiding this comment

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

... as you can hear, I'm leaning more to the former, but not a TS expert.

<Row key={key}>
<Cell>
<FormattedKey k={key} value={data[key]} />
</Cell>
<Cell>
<NestedValue
parentKey={key}
value={data[key]}
keySorter={keySorter}
depth={depth}
/>
</Cell>
</Row>
))}
</tbody>
</Table>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import React from 'react';
import { shallow, mount } from 'enzyme';
import 'jest-styled-components';
import {
NestedKeyValueTable,
NestedValue,
FormattedValue,
FormattedKey
} from '../NestedKeyValueTable';

describe('NestedKeyValueTable component', () => {
it('should render with data', () => {
const testData = {
a: 1,
b: 2,
c: [3, 4, 5],
d: { aa: 1, bb: 2 }
};
expect(shallow(<NestedKeyValueTable data={testData} />)).toMatchSnapshot();
});
it('should render an empty table if there is no data', () => {
expect(shallow(<NestedKeyValueTable data={{}} />)).toMatchSnapshot();
});
});

describe('NestedValue component', () => {
let props;

beforeEach(() => {
props = {
value: { a: 'hello' },
depth: 0,
keySorter: jest.fn(),
parentKey: 'who_cares'
};
});

it('should render a formatted value when depth is 0', () => {
expect(shallow(<NestedValue {...props} />)).toMatchSnapshot();
});

it('should render a formatted value when depth > 0 but value is not an object', () => {
props.value = 2;
props.depth = 3;
expect(shallow(<NestedValue {...props} />)).toMatchSnapshot();
});

it('should render a nested KV Table when depth > 0 and value is an object', () => {
props.depth = 1;
expect(shallow(<NestedValue {...props} />)).toMatchSnapshot();
});
});

describe('FormattedValue component', () => {
it('should render an object', () => {
expect(mount(<FormattedValue value={{ a: 'ok' }} />)).toMatchSnapshot();
});

it('should render an array', () => {
expect(mount(<FormattedValue value={[1, 2, 3]} />)).toMatchSnapshot();
});

it('should render a boolean', () => {
expect(mount(<FormattedValue value={true} />)).toMatchSnapshot();
expect(mount(<FormattedValue value={false} />)).toMatchSnapshot();
});

it('should render a number', () => {
expect(mount(<FormattedValue value={243} />)).toMatchSnapshot();
});

it('should render a string', () => {
expect(mount(<FormattedValue value="hey ok cool" />)).toMatchSnapshot();
});

it('should render null', () => {
expect(mount(<FormattedValue value={null} />)).toMatchSnapshot();
});

it('should render undefined', () => {
let b;
expect(mount(<FormattedValue value={b} />)).toMatchSnapshot();
expect(mount(<FormattedValue />)).toMatchSnapshot();
});
});

describe('FormattedKey component', () => {
it('should render when the value is null or undefined', () => {
let nope;
expect(mount(<FormattedKey k="testKey" value={null} />)).toMatchSnapshot();
expect(mount(<FormattedKey k="testKey" value={nope} />)).toMatchSnapshot();
expect(mount(<FormattedKey k="testKey" />)).toMatchSnapshot();
});

it('should render when the value is defined', () => {
expect(mount(<FormattedKey k="testKey" value="hi" />)).toMatchSnapshot();
expect(mount(<FormattedKey k="testKey" value={123} />)).toMatchSnapshot();
expect(mount(<FormattedKey k="testKey" value={{}} />)).toMatchSnapshot();
});
});
Loading