From c82f8439d3509e8d87b627091750bc9a21cab6d5 Mon Sep 17 00:00:00 2001 From: "Yuan (Bob) Gong" Date: Tue, 22 Oct 2019 12:31:20 +0800 Subject: [PATCH] Fix CustomTable.tsx layout problems (#2444) * Reformat files I will touch first * Refactor CustomTable: extract selection section components * Fix CustomTable layout issues for all combinations of expand button, radio button and disable selection options * Fix snapshot tests * Fix lint warnings --- frontend/src/components/CustomTable.tsx | 338 ++++++++++++------ frontend/src/components/CustomTableRow.tsx | 29 +- .../__snapshots__/CustomTable.test.tsx.snap | 62 +++- frontend/src/pages/PipelineList.tsx | 65 ++-- .../pages/__snapshots__/RunList.test.tsx.snap | 7 + 5 files changed, 341 insertions(+), 160 deletions(-) diff --git a/frontend/src/components/CustomTable.tsx b/frontend/src/components/CustomTable.tsx index 804f0411d5d..cf4252c306a 100644 --- a/frontend/src/components/CustomTable.tsx +++ b/frontend/src/components/CustomTable.tsx @@ -89,6 +89,9 @@ export const css = stylesheet({ fontWeight: 'bold', letterSpacing: 0.25, marginRight: 20, + overflow: 'hidden', + textOverflow: 'ellipsis', + whiteSpace: 'nowrap', }, emptyMessage: { padding: 20, @@ -165,7 +168,7 @@ export const css = stylesheet({ }, selectionToggle: { marginRight: 12, - minWidth: 32, + overflow: 'initial', // Resets overflow from 'hidden' }, verticalAlignInitial: { verticalAlign: 'initial', @@ -206,8 +209,10 @@ interface CustomTableState { export default class CustomTable extends React.Component { private _isMounted = true; - private _debouncedFilterRequest = - debounce((filterString: string) => this._requestFilter(filterString), 300); + private _debouncedFilterRequest = debounce( + (filterString: string) => this._requestFilter(filterString), + 300, + ); constructor(props: CustomTableProps) { super(props); @@ -219,8 +224,8 @@ export default class CustomTable extends React.Component v.id) : []; + const selectedIds = (event.target as CheckboxProps).checked + ? this.props.rows.map(v => v.id) + : []; if (this.props.updateSelection) { this.props.updateSelection(selectedIds); } @@ -249,9 +255,10 @@ export default class CustomTable extends React.Component total += (c.flex || 1), 0); - const widths = this.props.columns.map(c => (c.flex || 1) / totalFlex * 100); + const totalFlex = this.props.columns.reduce((total, c) => (total += c.flex || 1), 0); + const widths = this.props.columns.map(c => ((c.flex || 1) / totalFlex) * 100); return (
- {/* Filter/Search bar */} {!this.props.noFilterBox && (
- - ) - }} /> + ), + }} + />
)} {/* Header */} -
- {(this.props.disableSelection !== true && this.props.useRadioButtons !== true) && ( -
- -
- )} - {/* Shift cells to account for expand button */} - {!!this.props.getExpandComponent && ( - - )} +
+ {// Called as function to avoid breaking shallow rendering tests. + HeaderRowSelectionSection({ + disableSelection: this.props.disableSelection, + indeterminate: !!numSelected && numSelected < this.props.rows.length, + isSelected: !!numSelected && numSelected === this.props.rows.length, + onSelectAll: this.handleSelectAllClick.bind(this), + showExpandButton: !!this.props.getExpandComponent, + useRadioButtons: this.props.useRadioButtons, + })} {this.props.columns.map((col, i) => { const isColumnSortable = !!this.props.columns[i].sortKey; const isCurrentSortColumn = sortBy === this.props.columns[i].sortKey; return ( -
- {this.props.disableSorting === true &&
{col.label}
} +
+ {this.props.disableSorting === true && col.label} {!this.props.disableSorting && ( - - + this._requestSort(this.props.columns[i].sortKey)}> + onClick={() => this._requestSort(this.props.columns[i].sortKey)} + > {col.label} @@ -343,11 +366,16 @@ export default class CustomTable extends React.Component {/* Busy experience */} - {this.state.isBusy && ( -
- - )} + {this.state.isBusy && ( + +
+ + + )} {/* Empty experience */} {this.props.rows.length === 0 && !!this.props.emptyMessage && !this.state.isBusy && ( @@ -358,49 +386,42 @@ export default class CustomTable extends React.Component -
this.handleClick(e, row.id)}> - {/* Expansion toggle button */} - {((this.props.disableSelection !== true || !!this.props.getExpandComponent) && row.expandState !== ExpandState.NONE) && ( -
- {/* If using checkboxes */} - {(this.props.disableSelection !== true && this.props.useRadioButtons !== true) && ( - )} - {/* If using radio buttons */} - {(this.props.disableSelection !== true && this.props.useRadioButtons) && ( - )} - {!!this.props.getExpandComponent && ( - this._expandButtonToggled(e, i)}> - - - )} -
+ return ( +
+ key={i} + > +
this.handleClick(e, row.id)} + > + {// Called as function to avoid breaking shallow rendering tests. + BodyRowSelectionSection({ + disableSelection: this.props.disableSelection, + expandState: row.expandState, + isSelected: this.isSelected(row.id), + onExpand: e => this._expandButtonToggled(e, i), + showExpandButton: !!this.props.getExpandComponent, + useRadioButtons: this.props.useRadioButtons, + })} + +
+ {row.expandState === ExpandState.EXPANDED && this.props.getExpandComponent && ( +
{this.props.getExpandComponent(i)}
)} - - {}
- {row.expandState === ExpandState.EXPANDED && this.props.getExpandComponent && ( -
- {this.props.getExpandComponent(i)} -
- )} -
); + ); })}
@@ -408,20 +429,29 @@ export default class CustomTable extends React.Component Rows per page: - + InputProps={{ disableUnderline: true }} + onChange={this._requestRowsPerPage.bind(this)} + value={pageSize} + > {[10, 20, 50, 100].map((size, i) => ( - {size} + + {size} + ))} this._pageChanged(-1)} disabled={!this.state.currentPage}> - this._pageChanged(1)} - disabled={this.state.currentPage >= this.state.maxPageIndex}> + this._pageChanged(1)} + disabled={this.state.currentPage >= this.state.maxPageIndex} + >
@@ -432,13 +462,16 @@ export default class CustomTable extends React.Component { // Override the current state with incoming request - const request: ListRequest = Object.assign({ - filter: this.state.filterStringEncoded, - orderAscending: this.state.sortOrder === 'asc', - pageSize: this.state.pageSize, - pageToken: this.state.tokenList[this.state.currentPage], - sortBy: this.state.sortBy, - }, loadRequest); + const request: ListRequest = Object.assign( + { + filter: this.state.filterStringEncoded, + orderAscending: this.state.sortOrder === 'asc', + pageSize: this.state.pageSize, + pageToken: this.state.tokenList[this.state.currentPage], + sortBy: this.state.sortBy, + }, + loadRequest, + ); let result = ''; try { @@ -466,9 +499,9 @@ export default class CustomTable extends React.Component await this._debouncedFilterRequest(value as string) + async () => await this._debouncedFilterRequest(value as string), ); - } + }; // Exposed for testing protected async _requestFilter(filterString?: string): Promise { @@ -479,12 +512,14 @@ export default class CustomTable extends React.Component { this._resetToFirstPage( - await this.reload({ pageToken: '', orderAscending: sortOrder === 'asc', sortBy })); + await this.reload({ pageToken: '', orderAscending: sortOrder === 'asc', sortBy }), + ); }); } } @@ -563,3 +597,91 @@ export default class CustomTable extends React.Component = ({ + disableSelection, + indeterminate, + isSelected, + onSelectAll, + showExpandButton, + useRadioButtons, +}) => { + const nonEmpty = disableSelection !== true || showExpandButton; + if (!nonEmpty) { + return null; + } + + return ( +
+ {/* If using checkboxes */} + {disableSelection !== true && useRadioButtons !== true && ( + + )} + {/* If using radio buttons */} + {disableSelection !== true && useRadioButtons && ( + // Placeholder for radio button horizontal space. + + )} + {showExpandButton && } +
+ ); +}; + +interface BodyRowSelectionSectionProps extends SelectionSectionCommonProps { + expandState?: ExpandState; + onExpand: React.MouseEventHandler; +} +const BodyRowSelectionSection: React.FC = ({ + disableSelection, + expandState, + isSelected, + onExpand, + showExpandButton, + useRadioButtons, +}) => ( + <> + {/* Expansion toggle button */} + {(disableSelection !== true || showExpandButton) && expandState !== ExpandState.NONE && ( +
+ {/* If using checkboxes */} + {disableSelection !== true && useRadioButtons !== true && ( + + )} + {/* If using radio buttons */} + {disableSelection !== true && useRadioButtons && ( + + )} + {showExpandButton && ( + + + + )} +
+ )} + + {/* Placeholder for non-expandable rows */} + {expandState === ExpandState.NONE &&
} + +); diff --git a/frontend/src/components/CustomTableRow.tsx b/frontend/src/components/CustomTableRow.tsx index 4af1e185283..773ee031597 100644 --- a/frontend/src/components/CustomTableRow.tsx +++ b/frontend/src/components/CustomTableRow.tsx @@ -66,8 +66,8 @@ interface CustomTableRowProps { } function calculateColumnWidths(columns: Column[]): number[] { - const totalFlex = columns.reduce((total, c) => total += (c.flex || 1), 0); - return columns.map(c => (c.flex || 1) / totalFlex * 100); + const totalFlex = columns.reduce((total, c) => (total += c.flex || 1), 0); + return columns.map(c => ((c.flex || 1) / totalFlex) * 100); } // tslint:disable-next-line:variable-name @@ -76,17 +76,18 @@ export const CustomTableRow: React.FC = (props: CustomTable const widths = calculateColumnWidths(columns); return ( - { - row.otherFields.map((cell, i) => ( -
- {i === 0 && row.error && ( - - )} - {columns[i].customRenderer ? - columns[i].customRenderer!({ value: cell, id: row.id }) : cell} -
- )) - } + {row.otherFields.map((cell, i) => ( +
+ {i === 0 && row.error && ( + + + + )} + {columns[i].customRenderer + ? columns[i].customRenderer!({ value: cell, id: row.id }) + : cell} +
+ ))} ); -}; \ No newline at end of file +}; diff --git a/frontend/src/components/__snapshots__/CustomTable.test.tsx.snap b/frontend/src/components/__snapshots__/CustomTable.test.tsx.snap index 5f0df2ad04f..f97473f2417 100644 --- a/frontend/src/components/__snapshots__/CustomTable.test.tsx.snap +++ b/frontend/src/components/__snapshots__/CustomTable.test.tsx.snap @@ -55,11 +55,11 @@ exports[`CustomTable renders a collapsed row 1`] = ` indeterminate={false} onChange={[Function]} /> +
-
- +
+ +
-
- col1 -
+ col1
-
- col2 -
+ col2
+
-
{ () => this.state.selectedIds, 'pipeline', ids => this._selectionChanged(ids), - false, /* useCurrentResource */ + false /* useCurrentResource */, ) .getToolbarActionMap(), breadcrumbs: [], @@ -82,7 +82,7 @@ class PipelineList extends Page<{}, PipelineListState> { { label: 'Uploaded on', sortKey: PipelineSortKeys.CREATED_AT, flex: 1 }, ]; - const rows: Row[] = this.state.pipelines.map((p) => { + const rows: Row[] = this.state.pipelines.map(p => { return { id: p.id!, otherFields: [p.name!, p.description!, formatDateString(p.created_at!)], @@ -91,13 +91,22 @@ class PipelineList extends Page<{}, PipelineListState> { return (
- - - + + +
); } @@ -112,7 +121,11 @@ class PipelineList extends Page<{}, PipelineListState> { let response: ApiListPipelinesResponse | null = null; try { response = await Apis.pipelineServiceApi.listPipelines( - request.pageToken, request.pageSize, request.sortBy, request.filter); + request.pageToken, + request.pageSize, + request.sortBy, + request.filter, + ); this.clearBanner(); } catch (err) { await this.showPageError('Error: failed to retrieve list of pipelines.', err); @@ -123,14 +136,19 @@ class PipelineList extends Page<{}, PipelineListState> { return response ? response.next_page_token || '' : ''; } - private _nameCustomRenderer: React.FC> = (props: CustomRendererProps) => { + private _nameCustomRenderer: React.FC> = ( + props: CustomRendererProps, + ) => { return ( - e.stopPropagation()} + e.stopPropagation()} className={commonCss.link} - to={RoutePage.PIPELINE_DETAILS.replace(':' + RouteParams.pipelineId, props.id)}>{props.value} + to={RoutePage.PIPELINE_DETAILS.replace(':' + RouteParams.pipelineId, props.id)} + > + {props.value} ); - } + }; private _selectionChanged(selectedIds: string[]): void { const actions = this.props.toolbarProps.actions; @@ -139,12 +157,19 @@ class PipelineList extends Page<{}, PipelineListState> { this.setStateSafe({ selectedIds }); } - private async _uploadDialogClosed(confirmed: boolean, name: string, file: File | null, url: string, - method: ImportMethod, description?: string): Promise { - - if (!confirmed - || (method === ImportMethod.LOCAL && !file) - || (method === ImportMethod.URL && !url)) { + private async _uploadDialogClosed( + confirmed: boolean, + name: string, + file: File | null, + url: string, + method: ImportMethod, + description?: string, + ): Promise { + if ( + !confirmed || + (method === ImportMethod.LOCAL && !file) || + (method === ImportMethod.URL && !url) + ) { this.setStateSafe({ uploadDialogOpen: false }); return false; } diff --git a/frontend/src/pages/__snapshots__/RunList.test.tsx.snap b/frontend/src/pages/__snapshots__/RunList.test.tsx.snap index e80c1cb38ab..18f79c94491 100644 --- a/frontend/src/pages/__snapshots__/RunList.test.tsx.snap +++ b/frontend/src/pages/__snapshots__/RunList.test.tsx.snap @@ -2011,6 +2011,7 @@ exports[`RunList reloads the run when refresh is called 1`] = ` "width": "25%", } } + title="Run name" >