Skip to content

Commit

Permalink
feat: Improve Nested Column Types UI (amundsen-io#627)
Browse files Browse the repository at this point in the history
* feat: Create ColumnType component (amundsen-io#604)

* Create ColumnType component

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Update Modal UI

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Code cleanup; Add a test file

* Prevent ColumnListItem expand/collapse from being triggered

* Lint fix

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* feat: Parse column types (amundsen-io#611)

* WIP: Create a parser & render parsed text

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Cleanup logic

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* More cleanup

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Lint fix

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Code cleanup

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Code cleanup

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Code cleanup

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Use more appropriate elements; Fix typo

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Parser tests

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Fix button; Fix test; Remove obsolete style

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Fix duplicate test name

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* style: Improve UI styles and interactions (amundsen-io#617)

* Vertically center modal

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Match design font specifications

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Miscellaneous cleanup

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Use variables

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* test: Improves unit tests for ColumnType + QA fixes (amundsen-io#625)

* Parser tests

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Updates from design qa

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Improve ColumnType tests

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* log support

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Code cleanup

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Fix some lint warning

Signed-off-by: Tamika Tannis <ttannis@lyft.com>

* Betterer update

Signed-off-by: Tamika Tannis <ttannis@lyft.com>
  • Loading branch information
ttannis authored Aug 31, 2020
1 parent db72c14 commit fc15726
Show file tree
Hide file tree
Showing 15 changed files with 720 additions and 94 deletions.
38 changes: 19 additions & 19 deletions frontend/amundsen_application/static/.betterer.results
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,11 @@ exports[`strict null compilation`] = {
"js/components/SearchPage/index.tsx:1421221531": [
[175, 11, 12, "Property \'filterSections\' is missing in type \'{}\' but required in type \'Readonly<Pick<StateFromProps, \\"filterSections\\">>\'.", "250899467"]
],
"js/components/TableDetail/ColumnList/index.tsx:2024292996": [
[20, 6, 7, "Object is possibly \'undefined\'.", "3718923584"],
[25, 21, 7, "Object is possibly \'undefined\'.", "3718923584"],
[30, 6, 8, "Type \'string | undefined\' is not assignable to type \'string\'.\\n Type \'undefined\' is not assignable to type \'string\'.", "1427606500"],
[31, 6, 7, "Type \'string | undefined\' is not assignable to type \'string\'.\\n Type \'undefined\' is not assignable to type \'string\'.", "3817619378"]
"js/components/TableDetail/ColumnList/index.tsx:355148301": [
[22, 6, 7, "Object is possibly \'undefined\'.", "3718923584"],
[27, 21, 7, "Object is possibly \'undefined\'.", "3718923584"],
[33, 6, 8, "Type \'string | undefined\' is not assignable to type \'string\'.\\n Type \'undefined\' is not assignable to type \'string\'.", "1427606500"],
[34, 6, 7, "Type \'string | undefined\' is not assignable to type \'string\'.\\n Type \'undefined\' is not assignable to type \'string\'.", "3817619378"]
],
"js/components/TableDetail/ColumnStats/index.spec.tsx:1228258528": [
[90, 39, 4, "Argument of type \'null\' is not assignable to parameter of type \'number\'.", "2087897566"]
Expand All @@ -86,7 +86,7 @@ exports[`strict null compilation`] = {
[141, 17, 16, "Object is possibly \'undefined\'.", "3451845569"],
[257, 30, 34, "Argument of type \'number | null\' is not assignable to parameter of type \'number\'.\\n Type \'null\' is not assignable to type \'number\'.", "3967943985"]
],
"js/components/TableDetail/SourceLink/index.spec.tsx:3683846951": [
"js/components/TableDetail/SourceLink/index.spec.tsx:2646548231": [
[39, 21, 100, "Object is possibly \'null\'.", "1316242242"]
],
"js/components/TableDetail/TableDashboardResourceList/index.tsx:3147978263": [
Expand All @@ -107,19 +107,19 @@ exports[`strict null compilation`] = {
"js/components/TableDetail/index.spec.tsx:2169788066": [
[32, 4, 8, "Argument of type \'Partial<Location<{} | null | undefined>> | undefined\' is not assignable to parameter of type \'Partial<Location<{} | null | undefined>>\'.\\n Type \'undefined\' is not assignable to type \'Partial<Location<{} | null | undefined>>\'.", "2700611480"]
],
"js/components/TableDetail/index.tsx:1105490806": [
"js/components/TableDetail/index.tsx:3223260709": [
[153, 10, 13, "Type \'null\' is not assignable to type \'((newValue: string, onSuccess?: (() => any) | undefined, onFailure?: (() => any) | undefined) => void) | undefined\'.", "67794331"],
[164, 6, 7, "Type \'Element\' is not assignable to type \'never\'.", "3716929964"],
[171, 6, 3, "Type \'string\' is not assignable to type \'never\'.", "193424690"],
[172, 6, 5, "Type \'string\' is not assignable to type \'never\'.", "183222373"],
[182, 8, 7, "Type \'Element\' is not assignable to type \'never\'.", "3716929964"],
[183, 11, 26, "Type \'{ itemsPerPage: number; source: string; }\' is missing the following properties from type \'Readonly<Pick<TableDashboardResourceListProps, \\"source\\" | \\"isLoading\\" | \\"dashboards\\" | \\"errorText\\" | \\"itemsPerPage\\"> & OwnProps>\': isLoading, dashboards, errorText", "2224258167"],
[188, 8, 3, "Type \'string\' is not assignable to type \'never\'.", "193424690"],
[189, 8, 5, "Type \'string | Element\' is not assignable to type \'never\'.\\n Type \'string\' is not assignable to type \'never\'.", "183222373"],
[263, 16, 7, "Type \'string | null\' is not assignable to type \'string | undefined\'.\\n Type \'null\' is not assignable to type \'string | undefined\'.", "3817619378"],
[305, 20, 35, "Argument of type \'ProgrammaticDescription[] | undefined\' is not assignable to parameter of type \'ProgrammaticDescription[]\'.\\n Type \'undefined\' is not assignable to type \'ProgrammaticDescription[]\'.", "4249007202"],
[319, 20, 36, "Argument of type \'ProgrammaticDescription[] | undefined\' is not assignable to parameter of type \'ProgrammaticDescription[]\'.\\n Type \'undefined\' is not assignable to type \'ProgrammaticDescription[]\'.", "2770872537"],
[324, 16, 36, "Argument of type \'ProgrammaticDescription[] | undefined\' is not assignable to parameter of type \'ProgrammaticDescription[]\'.\\n Type \'undefined\' is not assignable to type \'ProgrammaticDescription[]\'.", "2776557981"]
[165, 6, 7, "Type \'Element\' is not assignable to type \'never\'.", "3716929964"],
[173, 6, 3, "Type \'string\' is not assignable to type \'never\'.", "193424690"],
[174, 6, 5, "Type \'string\' is not assignable to type \'never\'.", "183222373"],
[184, 8, 7, "Type \'Element\' is not assignable to type \'never\'.", "3716929964"],
[185, 11, 26, "Type \'{ itemsPerPage: number; source: string; }\' is missing the following properties from type \'Readonly<Pick<TableDashboardResourceListProps, \\"source\\" | \\"isLoading\\" | \\"dashboards\\" | \\"errorText\\" | \\"itemsPerPage\\"> & OwnProps>\': isLoading, dashboards, errorText", "2224258167"],
[190, 8, 3, "Type \'string\' is not assignable to type \'never\'.", "193424690"],
[191, 8, 5, "Type \'string | Element\' is not assignable to type \'never\'.\\n Type \'string\' is not assignable to type \'never\'.", "183222373"],
[265, 16, 7, "Type \'string | null\' is not assignable to type \'string | undefined\'.\\n Type \'null\' is not assignable to type \'string | undefined\'.", "3817619378"],
[307, 20, 35, "Argument of type \'ProgrammaticDescription[] | undefined\' is not assignable to parameter of type \'ProgrammaticDescription[]\'.\\n Type \'undefined\' is not assignable to type \'ProgrammaticDescription[]\'.", "4249007202"],
[321, 20, 36, "Argument of type \'ProgrammaticDescription[] | undefined\' is not assignable to parameter of type \'ProgrammaticDescription[]\'.\\n Type \'undefined\' is not assignable to type \'ProgrammaticDescription[]\'.", "2770872537"],
[326, 16, 36, "Argument of type \'ProgrammaticDescription[] | undefined\' is not assignable to parameter of type \'ProgrammaticDescription[]\'.\\n Type \'undefined\' is not assignable to type \'ProgrammaticDescription[]\'.", "2776557981"]
],
"js/components/common/Announcements/AnnouncementsList/index.spec.tsx:1395073325": [
[94, 23, 124, "Object is possibly \'null\'.", "4248337497"]
Expand Down Expand Up @@ -163,7 +163,7 @@ exports[`strict null compilation`] = {
"js/components/common/EntityCard/EntityCardSection/index.tsx:1592385405": [
[40, 4, 23, "Object is possibly \'null\'.", "1725552512"]
],
"js/components/common/Flag/index.tsx:128873066": [
"js/components/common/Flag/index.tsx:2997458704": [
[44, 27, 8, "Argument of type \'string | null\' is not assignable to parameter of type \'string\'.\\n Type \'null\' is not assignable to type \'string\'.", "4036080041"]
],
"js/components/common/OwnerEditor/index.spec.tsx:3936675418": [
Expand Down
8 changes: 8 additions & 0 deletions frontend/amundsen_application/static/css/_fonts-default.scss
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,14 @@

@import 'variables';

// Space Mono
@font-face {
font-family: 'Space Mono';
font-style: normal;
font-weight: $font-weight-header-regular;
src: url('/static/fonts/SpaceMono-Regular.ttf') format('truetype');
}

// Roboto
@font-face {
font-family: 'Roboto';
Expand Down
5 changes: 0 additions & 5 deletions frontend/amundsen_application/static/css/_popovers.scss
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,3 @@
margin: auto;
width: 24px;
}

.column-type-popover {
max-width: 552px; // arbitrary 2x default
word-break: break-word;
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ $font-weight-header-regular: 500 !default;
$font-weight-header-bold: 700 !default;

$font-family-monospace: 'Menlo-Bold', menlo, monospace !default;
$font-family-monospace-code: 'Space Mono', menlo, monospace !default;
$font-family-serif: georgia, 'Times New Roman', times, serif !default;

$font-size-small: 12px !default;
Expand Down Expand Up @@ -162,6 +163,8 @@ $w2-headline-line-height: 32px;
$w3-headline-font-size: 22px;
$w3-headline-line-height: 26px;

$code-font-size: 12px;

$title-font-weight: $font-weight-body-bold;
$subtitle-font-weight: $font-weight-body-semi-bold;
$body-font-weight: $font-weight-body-regular;
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@ import './styles.scss';

interface ColumnListProps {
columns?: TableColumn[];
database: string;
editText?: string;
editUrl?: string;
}

const ColumnList: React.FC<ColumnListProps> = ({
columns,
database,
editText,
editUrl,
}: ColumnListProps) => {
Expand All @@ -27,6 +29,7 @@ const ColumnList: React.FC<ColumnListProps> = ({
<ColumnListItem
key={`column:${index}`}
data={entry}
database={database}
index={index}
editText={editText}
editUrl={editUrl}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
// Copyright Contributors to the Amundsen project.
// SPDX-License-Identifier: Apache-2.0

import * as React from 'react';
import { mount } from 'enzyme';

import { Modal } from 'react-bootstrap';

import * as UtilMethods from 'ducks/utilMethods';
import ColumnType, { ColumnTypeProps } from '.';

const logClickSpy = jest.spyOn(UtilMethods, 'logClick');
logClickSpy.mockImplementation(() => null);

const setup = (propOverrides?: Partial<ColumnTypeProps>) => {
const props = {
columnName: 'test',
database: 'presto',
type:
'row(test_id varchar,test2 row(test2_id varchar,started_at timestamp,ended_at timestamp))',
...propOverrides,
};
const wrapper = mount<ColumnType>(<ColumnType {...props} />);
return { wrapper, props };
};
const { wrapper, props } = setup();

describe('ColumnType', () => {
describe('lifecycle', () => {
describe('when clicking on column-type-btn', () => {
it('should call showModal on the instance', () => {
const clickSpy = jest.spyOn(wrapper.instance(), 'showModal');
wrapper.instance().forceUpdate();
wrapper.find('.column-type-btn').simulate('click');

expect(clickSpy).toHaveBeenCalled();
});

it('should log the interaction', () => {
logClickSpy.mockClear();
wrapper.find('.column-type-btn').simulate('click');

expect(logClickSpy).toHaveBeenCalled();
});
});
});

describe('render', () => {
it('renders the column type string for simple types', () => {
const { wrapper, props } = setup({ type: 'varchar(32)' });
expect(wrapper.find('.column-type').text()).toBe(props.type);
});

describe('for nested types', () => {
it('renders the truncated column type string', () => {
const actual = wrapper.find('.column-type-btn').text();
const expected = 'row(...)';

expect(actual).toBe(expected);
});

describe('renders a modal', () => {
it('exists', () => {
const actual = wrapper.find(Modal).exists();
const expected = true;

expect(actual).toBe(expected);
});

it('renders props.type in modal body', () => {
const actual = wrapper.find('.sub-title').text();
const expected = props.columnName;

expect(actual).toBe(expected);
});

it('renders props.type in modal body', () => {
const actual = wrapper.find('.modal-body').text();
const expected = props.type;

expect(actual).toBe(expected);
});
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
// Copyright Contributors to the Amundsen project.
// SPDX-License-Identifier: Apache-2.0

import * as React from 'react';
import { Modal, OverlayTrigger, Popover } from 'react-bootstrap';

import { logClick } from 'ducks/utilMethods';

import './styles.scss';

import {
getTruncatedText,
parseNestedType,
NestedType,
ParsedType,
} from './parser';

const CTA_TEXT = 'Click to see nested fields';
const MODAL_TITLE = 'Nested Type';
const TEXT_INDENT = 8;

export interface ColumnTypeProps {
columnName: string;
database: string;
type: string;
}

export interface ColumnTypeState {
showModal: boolean;
}

export class ColumnType extends React.Component<
ColumnTypeProps,
ColumnTypeState
> {
nestedType: NestedType | null;

constructor(props) {
super(props);

this.state = {
showModal: false,
};
const { database, type } = this.props;
this.nestedType = parseNestedType(type, database);
}

hideModal = (e) => {
this.stopPropagation(e);
this.setState({ showModal: false });
};

showModal = (e) => {
logClick(e);
this.stopPropagation(e);
this.setState({ showModal: true });
};

stopPropagation = (e) => {
if (e) {
e.stopPropagation();
}
};

createLineItem = (text: string, textIndent: number) => {
return (
<div key={`lineitem:${text}`} style={{ textIndent: `${textIndent}px` }}>
{text}
</div>
);
};

renderParsedChildren = (children: ParsedType[], level: number) => {
const textIndent = level * TEXT_INDENT;
return children.map((item) => {
if (typeof item === 'string') {
return this.createLineItem(item, textIndent);
}
return this.renderNestedType(item, level);
});
};

renderNestedType = (nestedType: NestedType, level: number = 0) => {
const { head, tail, children } = nestedType;
const textIndent = level * TEXT_INDENT;
return (
<div key={`nesteditem:${head}${tail}`}>
{this.createLineItem(head, textIndent)}
{this.renderParsedChildren(children, level + 1)}
{this.createLineItem(tail, textIndent)}
</div>
);
};

render = () => {
const { columnName, type } = this.props;

if (this.nestedType === null) {
return <p className="column-type">{type}</p>;
}

const popoverHover = (
<Popover
className="column-type-popover"
id={`column-type-popover:${columnName}`}
>
{CTA_TEXT}
</Popover>
);
return (
<div onClick={this.stopPropagation}>
<OverlayTrigger
trigger={['hover', 'focus']}
placement="top"
overlay={popoverHover}
rootClose
>
<button
data-type="column-type"
type="button"
className="column-type-btn"
onClick={this.showModal}
>
{getTruncatedText(this.nestedType)}
</button>
</OverlayTrigger>
<Modal
className="column-type-modal"
show={this.state.showModal}
onHide={this.hideModal}
>
<Modal.Header closeButton>
<Modal.Title>
<h5 className="main-title">{MODAL_TITLE}</h5>
<div className="sub-title">{columnName}</div>
</Modal.Title>
</Modal.Header>
<Modal.Body>
<div className="column-type-modal-content">
{this.renderNestedType(this.nestedType)}
</div>
</Modal.Body>
</Modal>
</div>
);
};
}

export default ColumnType;
Loading

0 comments on commit fc15726

Please sign in to comment.