Skip to content

feat: Nested columns special type rows and expand by default #1872

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

Merged
merged 4 commits into from
May 26, 2022
Merged

feat: Nested columns special type rows and expand by default #1872

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e73362
Special type handling rows for nested columns, and expand nested by d…
kristenarmes May 24, 2022
895effc
Using constant for row height
kristenarmes May 24, 2022
6d798d5
Addressing PR comments
kristenarmes May 26, 2022
45e18ca
Passing max number of nested cols as an option to Table instead of fr…
kristenarmes May 26, 2022
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
42 changes: 20 additions & 22 deletions frontend/amundsen_application/static/.betterer.results
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,22 +65,22 @@ exports[`eslint`] = {
[118, 19, 20, "Must use destructuring state assignment", "3067028466"],
[126, 10, 137, "Visible, non-interactive elements with click handlers must have at least one keyboard listener.", "3965651236"]
],
"js/components/EditableText/index.tsx:2818784014": [
[50, 2, 21, "textAreaRef should be placed after componentDidUpdate", "3072052035"],
[71, 6, 13, "Do not use setState in componentDidUpdate", "57229240"],
[79, 10, 25, "Must use destructuring props assignment", "793704523"],
[80, 8, 25, "Must use destructuring props assignment", "793704523"],
[82, 32, 16, "Must use destructuring state assignment", "3998965439"],
[82, 53, 21, "Must use destructuring state assignment", "1159122654"],
[84, 6, 13, "Do not use setState in componentDidUpdate", "57229240"],
[89, 4, 22, "Must use destructuring props assignment", "2225424112"],
[93, 4, 22, "Must use destructuring props assignment", "2225424112"],
[97, 27, 23, "Must use destructuring props assignment", "2982501243"],
[100, 23, 23, "Must use destructuring props assignment", "2982501243"],
[108, 6, 22, "Must use destructuring props assignment", "2225424112"],
[115, 4, 24, "Must use destructuring props assignment", "3049746099"],
[131, 19, 20, "Script URL is a form of eval.", "3373049033"],
[143, 8, 207, "A control must be associated with a text label.", "2914986446"]
"js/components/EditableText/index.tsx:2168483193": [
[51, 2, 21, "textAreaRef should be placed after componentDidUpdate", "3072052035"],
[72, 6, 13, "Do not use setState in componentDidUpdate", "57229240"],
[80, 10, 25, "Must use destructuring props assignment", "793704523"],
[81, 8, 25, "Must use destructuring props assignment", "793704523"],
[83, 32, 16, "Must use destructuring state assignment", "3998965439"],
[83, 53, 21, "Must use destructuring state assignment", "1159122654"],
[85, 6, 13, "Do not use setState in componentDidUpdate", "57229240"],
[90, 4, 22, "Must use destructuring props assignment", "2225424112"],
[94, 4, 22, "Must use destructuring props assignment", "2225424112"],
[98, 27, 23, "Must use destructuring props assignment", "2982501243"],
[101, 23, 23, "Must use destructuring props assignment", "2982501243"],
[109, 6, 22, "Must use destructuring props assignment", "2225424112"],
[116, 4, 24, "Must use destructuring props assignment", "3049746099"],
[134, 19, 20, "Script URL is a form of eval.", "3373049033"],
[146, 8, 207, "A control must be associated with a text label.", "2914986446"]
],
"js/components/EntityCard/EntityCardSection/index.tsx:474926744": [
[25, 2, 55, "editButton should be placed after constructor", "2479426463"],
Expand Down Expand Up @@ -467,11 +467,9 @@ exports[`eslint`] = {
"js/features/ColumnList/ColumnStats/columnStats.story.tsx:2552692472": [
[9, 0, 48, "\`./testDataBuilder\` import should occur before import of \`.\`", "3767205268"]
],
"js/features/ColumnList/ColumnType/index.tsx:2472856984": [
[34, 2, 30, "nestedType should be placed after constructor", "1117758211"],
[105, 6, 36, "Visible, non-interactive elements with click handlers must have at least one keyboard listener.", "3801508926"],
[105, 6, 36, "Static HTML elements with event handlers require a role.", "3801508926"],
[123, 16, 20, "Must use destructuring state assignment", "2976153148"]
"js/features/ColumnList/ColumnType/index.tsx:1333623377": [
[106, 6, 36, "Visible, non-interactive elements with click handlers must have at least one keyboard listener.", "3801508926"],
[106, 6, 36, "Static HTML elements with event handlers require a role.", "3801508926"]
],
"js/features/ColumnList/ColumnType/parser.ts:4274243564": [
[58, 6, 10, "Assignment to function parameter \'startIndex\'.", "3807744539"],
Expand Down Expand Up @@ -681,7 +679,7 @@ exports[`eslint`] = {
"js/pages/TableDetailPage/WatermarkLabel/index.tsx:2189911402": [
[29, 22, 21, "Must use destructuring props assignment", "587844958"]
],
"js/pages/TableDetailPage/index.tsx:1654932239": [
"js/pages/TableDetailPage/index.tsx:953038643": [
[160, 2, 20, "key should be placed after componentDidUpdate", "3916788587"],
[206, 6, 13, "Do not use setState in componentDidUpdate", "57229240"]
],
Expand Down
6 changes: 6 additions & 0 deletions frontend/amundsen_application/static/css/_typography-default.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,12 @@ body {
color: $column-name-color;
}

.column-type-label {
@extend %text-title-w3;

color: $text-primary;
}

.resource-type {
@extend %text-subtitle-w3;

Expand Down
14 changes: 14 additions & 0 deletions frontend/amundsen_application/static/js/components/Table/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,17 @@

export const COLUMN_NAME_REGEX = '\\/(\\w+)'; // '/' followed by one or more word characters
export const TYPE_METADATA_REGEX = '\\/type\\/(\\w+)'; // '/type/' followed by one or more word characters

export const ARRAY_KIND = 'array';
export const MAP_KIND = 'map';
export const MAP_KEY_NAME = '_map_key';
export const MAP_VALUE_NAME = '_map_value';
export const MAP_KEY_DISPLAY_NAME = 'map key';
export const MAP_VALUE_DISPLAY_NAME = 'map value';

export const ARRAY_LABEL = 'array';
export const ARRAY_OPENER = ' [';
export const ARRAY_CLOSER = '] ';
export const MAP_LABEL = 'map';
export const MAP_OPENER = ' {';
export const MAP_CLOSER = '} ';
142 changes: 120 additions & 22 deletions frontend/amundsen_application/static/js/components/Table/index.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,21 @@ import Table, { TableProps } from '.';

const dataBuilder = new TestDataBuilder();

const formatChildrenDataMock = jest
.fn()
.mockImplementation((rowValue) => ({ key: rowValue.key }));
let mockMaxNestedColumns = 20;
jest.mock('config/config-utils', () => {
const configUtilsModule = jest.requireActual('config/config-utils');
return {
...configUtilsModule,
getMaxNestedColumns: () => mockMaxNestedColumns,
};
});
const formatChildrenDataMock = jest.fn().mockImplementation((rowValue) => ({
key: rowValue.key,
name: rowValue.name,
isExpandable: rowValue.isExpandable,
kind: rowValue.kind,
children: rowValue.children,
}));

const setup = (propOverrides?: Partial<TableProps>) => {
const { data, columns } = dataBuilder.build();
Expand Down Expand Up @@ -204,6 +216,68 @@ describe('Table', () => {
});
});
});

describe('when the data has nested children', () => {
const { columns, data } = dataBuilder.withCollapsedRow().build();

it('displays the expected specific type rows', () => {
mockMaxNestedColumns = 20;
const { wrapper } = setup({
data,
columns,
options: {
formatChildrenData: formatChildrenDataMock,
},
});

// The child rows include one array, one map, and one map within an array
// (multiplied by 2 for opener and closer)
const expected = 6;
const actual = wrapper.find('.is-specific-type-row').length;

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

describe('when the total amount of rows does not exceed the max configured value', () => {
mockMaxNestedColumns = 20;
it('expands all the children by default', () => {
const { wrapper } = setup({
data,
columns,
options: {
formatChildrenData: formatChildrenDataMock,
},
});

// 8 total child rows
const expected = data.length + 8;
const actual = wrapper
.find('.ams-table-body .ams-table-row')
.not('.is-specific-type-row').length;

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

describe('when the total amount of rows exceeds the max configured value', () => {
it('does not expand the children by default', () => {
mockMaxNestedColumns = 0;
const { wrapper } = setup({
data,
columns,
options: {
formatChildrenData: formatChildrenDataMock,
},
});

const expected = data.length;
const actual = wrapper.find('.ams-table-body .ams-table-row')
.length;

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

describe('columns', () => {
Expand Down Expand Up @@ -723,6 +797,7 @@ describe('Table', () => {
window.HTMLElement.prototype.scrollIntoView = jest.fn();

it('preexpands the row that corresponds to the key', () => {
mockMaxNestedColumns = 0;
const { wrapper } = setup({
data,
columns,
Expand All @@ -736,27 +811,32 @@ describe('Table', () => {

// The first row has two child rows when preexpanded
const expected = data.length + 2;
const actual = wrapper.find('.ams-table-body .ams-table-row').length;
const actual = wrapper
.find('.ams-table-body .ams-table-row')
.not('.is-specific-type-row').length;

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

it('preexpands the row and all the parent rows that correspond to the key', () => {
mockMaxNestedColumns = 0;
const { wrapper } = setup({
data,
columns,
options: {
tableKey: 'database://cluster.schema/table',
preExpandPanelKey:
'database://cluster.schema/table/rowName/type/rowName/col1',
'database://cluster.schema/table/rowName/type/rowName/_inner_/col2/_map_value',
preExpandRightPanel: preExpandRightPanelSpy,
formatChildrenData: formatChildrenDataMock,
},
});

// The first row has two child rows when preexpanded
const expected = data.length + 2;
const actual = wrapper.find('.ams-table-body .ams-table-row').length;
// The first row has four child rows when preexpanded
const expected = data.length + 4;
const actual = wrapper
.find('.ams-table-body .ams-table-row')
.not('.is-specific-type-row').length;

expect(actual).toEqual(expected);
});
Expand All @@ -765,74 +845,84 @@ describe('Table', () => {
});

describe('lifetime', () => {
describe('when expanding and collapsing rows', () => {
describe('when collapsing and expanding rows', () => {
const { columns, data } = dataBuilder.withCollapsedRow().build();
beforeAll(() => {
// Expand the child rows by default
mockMaxNestedColumns = 20;
});

describe('when clicking on expand button', () => {
it('shows the expanded rows', () => {
describe('when clicking on collapse button', () => {
it('hide the expanded rows', () => {
const { wrapper } = setup({
data,
columns,
options: {
formatChildrenData: formatChildrenDataMock,
},
});
// The first row has two child rows when expanded
const expected = data.length + 2;
// The other rows have 4 child rows still expanded
const expected = data.length + 4;

wrapper
.find('.ams-table-body .ams-table-expanding-button')
.at(0)
.simulate('click');

const actual = wrapper.find('.ams-table-body .ams-table-row').length;
const actual = wrapper
.find('.ams-table-body .ams-table-row')
.not('.is-specific-type-row').length;

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

describe('when clicking again', () => {
it('hides the expand row', () => {
it('shows the expanded rows', () => {
const { wrapper } = setup({
data,
columns,
options: {
formatChildrenData: formatChildrenDataMock,
},
});
const expected = data.length;
// 8 total child rows
const expected = data.length + 8;

wrapper
.find('.ams-table-body .ams-table-expanding-button')
.at(0)
.simulate('click')
.simulate('click');

const actual = wrapper.find('.ams-table-body .ams-table-row')
.length;
const actual = wrapper
.find('.ams-table-body .ams-table-row')
.not('.is-specific-type-row').length;

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

describe('when clicking on multiple expand buttons', () => {
it('shows all the expanded rows', () => {
describe('when clicking on multiple collapse buttons', () => {
it('hides all the expanded rows', () => {
const { wrapper } = setup({
data,
columns,
options: {
formatChildrenData: formatChildrenDataMock,
},
});
// The first two rows have four total child rows when both expanded
const expected = data.length + 4;
// All child rows are collapsed
const expected = data.length;

wrapper
.find('.ams-table-body .ams-table-expanding-button')
.not('.is-specific-type-row')
.at(0)
.simulate('click');
wrapper
.find('.ams-table-body .ams-table-expanding-button')
.not('.is-specific-type-row')
.at(1)
.simulate('click');

Expand All @@ -845,6 +935,10 @@ describe('Table', () => {

describe('when onExpand is passed', () => {
const { columns, data } = dataBuilder.withCollapsedRow().build();
beforeAll(() => {
// Collapse the child rows by default
mockMaxNestedColumns = 0;
});
describe('when clicking on expand button', () => {
it('calls the onExpand handler', () => {
const onExpandSpy = jest.fn();
Expand Down Expand Up @@ -949,6 +1043,10 @@ describe('Table', () => {

describe('when onCollapse is passed', () => {
const { columns, data } = dataBuilder.withCollapsedRow().build();
beforeAll(() => {
// Collapse the child rows by default
mockMaxNestedColumns = 0;
});

describe('when clicking on expand button', () => {
it('does not call the onCollapse handler', () => {
Expand Down
Loading