From faf8588097d894da5e883d323874d93a77c4ef4e Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Fri, 23 Apr 2021 09:28:45 -0700 Subject: [PATCH] Minor improvements Summary: Some styling fixes and minor improvements in DataTable, used by network plugin: - be able to customise the context menu - be able to customise how entire rows are copied and presented on the clipboard to be able to deviate from the standard JSON - deeplink handling was made async, this gives the plugin the opportunity to first handle initial setup and rendering before trying to jump somewhere which is a typical use case for deeplinking Reviewed By: passy Differential Revision: D27947186 fbshipit-source-id: a56f081d60520c4bc2ad3c547a8ca5b9357e71a1 --- .../src/__tests__/PluginContainer.node.tsx | 10 +++++++++ .../src/__tests__/test-utils.node.tsx | 6 ++--- .../flipper-plugin/src/plugin/PluginBase.tsx | 9 +++++++- .../src/test-utils/test-utils.tsx | 9 ++++++-- .../flipper-plugin/src/ui/DataFormatter.tsx | 2 +- .../src/ui/data-table/DataTable.tsx | 18 +++++++++++++-- .../src/ui/data-table/DataTableManager.tsx | 22 +++++++++++++++++++ .../src/ui/data-table/TableContextMenu.tsx | 18 ++++++++++----- .../src/ui/data-table/TableHead.tsx | 4 ++++ .../src/ui/data-table/TableRow.tsx | 2 +- .../data-table/__tests__/DataTable.node.tsx | 16 +++++++------- 11 files changed, 92 insertions(+), 24 deletions(-) diff --git a/desktop/app/src/__tests__/PluginContainer.node.tsx b/desktop/app/src/__tests__/PluginContainer.node.tsx index 428f2d697..6c41b735d 100644 --- a/desktop/app/src/__tests__/PluginContainer.node.tsx +++ b/desktop/app/src/__tests__/PluginContainer.node.tsx @@ -24,6 +24,7 @@ import { import {selectPlugin} from '../reducers/connections'; import {updateSettings} from '../reducers/settings'; import {switchPlugin} from '../reducers/pluginManager'; +import {sleep} from 'flipper-plugin/src/utils/sleep'; interface PersistedState { count: 1; @@ -528,6 +529,7 @@ test('PluginContainer + Sandy plugin supports deeplink', async () => { ); }); + await sleep(10); expect(linksSeen).toEqual(['universe!']); expect(renderer.baseElement).toMatchInlineSnapshot(` @@ -558,6 +560,7 @@ test('PluginContainer + Sandy plugin supports deeplink', async () => { }), ); }); + await sleep(10); expect(linksSeen).toEqual(['universe!']); // ...nor does a random other store update that does trigger a plugin container render @@ -580,6 +583,7 @@ test('PluginContainer + Sandy plugin supports deeplink', async () => { }), ); }); + await sleep(10); expect(linksSeen).toEqual(['universe!', 'london!']); // and same link does trigger if something else was selected in the mean time @@ -601,6 +605,7 @@ test('PluginContainer + Sandy plugin supports deeplink', async () => { }), ); }); + await sleep(10); expect(linksSeen).toEqual(['universe!', 'london!', 'london!']); }); @@ -802,6 +807,7 @@ test('PluginContainer + Sandy device plugin supports deeplink', async () => { ); }); + await sleep(10); expect(linksSeen).toEqual([theUniverse]); expect(renderer.baseElement).toMatchInlineSnapshot(` @@ -832,6 +838,7 @@ test('PluginContainer + Sandy device plugin supports deeplink', async () => { }), ); }); + await sleep(10); expect(linksSeen).toEqual([theUniverse]); // ...nor does a random other store update that does trigger a plugin container render @@ -854,6 +861,7 @@ test('PluginContainer + Sandy device plugin supports deeplink', async () => { }), ); }); + await sleep(10); expect(linksSeen).toEqual([theUniverse, 'london!']); // and same link does trigger if something else was selected in the mean time @@ -875,6 +883,7 @@ test('PluginContainer + Sandy device plugin supports deeplink', async () => { }), ); }); + await sleep(10); expect(linksSeen).toEqual([theUniverse, 'london!', 'london!']); }); @@ -977,6 +986,7 @@ test('Sandy plugins support isPluginSupported + selectPlugin', async () => { pluginInstance.selectPlugin(definition.id, 'data'); expect(store.getState().connections.selectedPlugin).toBe(definition.id); expect(pluginInstance.activatedStub).toBeCalledTimes(2); + await sleep(10); expect(renderer.baseElement.querySelector('h1')).toMatchInlineSnapshot(`

Plugin1 diff --git a/desktop/flipper-plugin/src/__tests__/test-utils.node.tsx b/desktop/flipper-plugin/src/__tests__/test-utils.node.tsx index 17ab38cf0..4edd3abc0 100644 --- a/desktop/flipper-plugin/src/__tests__/test-utils.node.tsx +++ b/desktop/flipper-plugin/src/__tests__/test-utils.node.tsx @@ -402,7 +402,7 @@ test('plugins can receive deeplinks', async () => { }); expect(plugin.instance.field1.get()).toBe(''); - plugin.triggerDeepLink('test'); + await plugin.triggerDeepLink('test'); expect(plugin.instance.field1.get()).toBe('test'); }); @@ -424,7 +424,7 @@ test('device plugins can receive deeplinks', async () => { }); expect(plugin.instance.field1.get()).toBe(''); - plugin.triggerDeepLink('test'); + await plugin.triggerDeepLink('test'); expect(plugin.instance.field1.get()).toBe('test'); }); @@ -455,7 +455,7 @@ test('plugins can register menu entries', async () => { }); expect(plugin.instance.counter.get()).toBe(0); - plugin.triggerDeepLink('test'); + await plugin.triggerDeepLink('test'); plugin.triggerMenuEntry('createPaste'); plugin.triggerMenuEntry('Custom Action'); expect(plugin.instance.counter.get()).toBe(4); diff --git a/desktop/flipper-plugin/src/plugin/PluginBase.tsx b/desktop/flipper-plugin/src/plugin/PluginBase.tsx index 182c6ccf6..1aca92c44 100644 --- a/desktop/flipper-plugin/src/plugin/PluginBase.tsx +++ b/desktop/flipper-plugin/src/plugin/PluginBase.tsx @@ -325,7 +325,14 @@ export abstract class BasePluginInstance { this.assertNotDestroyed(); if (deepLink !== this.lastDeeplink) { this.lastDeeplink = deepLink; - this.events.emit('deeplink', deepLink); + if (typeof setImmediate !== 'undefined') { + // we only want to trigger deeplinks after the plugin had a chance to render + setImmediate(() => { + this.events.emit('deeplink', deepLink); + }); + } else { + this.events.emit('deeplink', deepLink); + } } } diff --git a/desktop/flipper-plugin/src/test-utils/test-utils.tsx b/desktop/flipper-plugin/src/test-utils/test-utils.tsx index 03a8a95e4..dad9bf6be 100644 --- a/desktop/flipper-plugin/src/test-utils/test-utils.tsx +++ b/desktop/flipper-plugin/src/test-utils/test-utils.tsx @@ -95,7 +95,7 @@ interface BasePluginResult { /** * Emulate triggering a deeplink */ - triggerDeepLink(deeplink: unknown): void; + triggerDeepLink(deeplink: unknown): Promise; /** * Grab all the persistable state, but will ignore any onExport handler @@ -386,8 +386,13 @@ function createBasePluginResult( exportStateAsync: () => pluginInstance.exportState(createStubIdler(), () => {}), exportState: () => pluginInstance.exportStateSync(), - triggerDeepLink: (deepLink: unknown) => { + triggerDeepLink: async (deepLink: unknown) => { pluginInstance.triggerDeepLink(deepLink); + return new Promise((resolve) => { + // this ensures the test won't continue until the setImmediate used by + // the deeplink handling event is handled + setImmediate(resolve); + }); }, destroy: () => pluginInstance.destroy(), triggerMenuEntry: (action: string) => { diff --git a/desktop/flipper-plugin/src/ui/DataFormatter.tsx b/desktop/flipper-plugin/src/ui/DataFormatter.tsx index 5dd36c16a..da4f84660 100644 --- a/desktop/flipper-plugin/src/ui/DataFormatter.tsx +++ b/desktop/flipper-plugin/src/ui/DataFormatter.tsx @@ -49,7 +49,7 @@ export const DataFormatter = { return ( value.toTimeString().split(' ')[0] + '.' + - pad('' + value.getMilliseconds(), 3) + pad('' + value.getMilliseconds(), 3, '0') ); } if (value instanceof Map) { diff --git a/desktop/flipper-plugin/src/ui/data-table/DataTable.tsx b/desktop/flipper-plugin/src/ui/data-table/DataTable.tsx index eee940748..5adc2ec03 100644 --- a/desktop/flipper-plugin/src/ui/data-table/DataTable.tsx +++ b/desktop/flipper-plugin/src/ui/data-table/DataTable.tsx @@ -57,6 +57,8 @@ interface DataTableProps { // multiselect?: true tableManagerRef?: RefObject | undefined>; // Actually we want a MutableRefObject, but that is not what React.createRef() returns, and we don't want to put the burden on the plugin dev to cast it... _testHeight?: number; // exposed for unit testing only + onCopyRows?(records: T[]): string; + onContextMenu?: (selection: undefined | T) => React.ReactElement; } export type DataTableColumn = { @@ -94,11 +96,13 @@ export interface RenderContext { export function DataTable( props: DataTableProps, ): React.ReactElement { - const {dataSource, onRowStyle, onSelect} = props; + const {dataSource, onRowStyle, onSelect, onCopyRows, onContextMenu} = props; useAssertStableRef(dataSource, 'dataSource'); useAssertStableRef(onRowStyle, 'onRowStyle'); useAssertStableRef(props.onSelect, 'onRowSelect'); useAssertStableRef(props.columns, 'columns'); + useAssertStableRef(onCopyRows, 'onCopyRows'); + useAssertStableRef(onContextMenu, 'onContextMenu'); useAssertStableRef(props._testHeight, '_testHeight'); // lint disabled for conditional inclusion of a hook (_testHeight is asserted to be stable) @@ -357,8 +361,18 @@ export function DataTable( selection, tableState.columns, visibleColumns, + onCopyRows, + onContextMenu, ), - [dataSource, dispatch, selection, tableState.columns, visibleColumns], + [ + dataSource, + dispatch, + selection, + tableState.columns, + visibleColumns, + onCopyRows, + onContextMenu, + ], ); useEffect(function initialSetup() { diff --git a/desktop/flipper-plugin/src/ui/data-table/DataTableManager.tsx b/desktop/flipper-plugin/src/ui/data-table/DataTableManager.tsx index 657b01f0c..a207a64a4 100644 --- a/desktop/flipper-plugin/src/ui/data-table/DataTableManager.tsx +++ b/desktop/flipper-plugin/src/ui/data-table/DataTableManager.tsx @@ -62,6 +62,13 @@ type DataManagerActions = addToSelection?: boolean; } > + | Action< + 'selectItemById', + { + id: string | number; + addToSelection?: boolean; + } + > | Action< 'addRangeToSelection', { @@ -161,6 +168,17 @@ export const dataTableManagerReducer = produce< ); break; } + case 'selectItemById': { + const {id, addToSelection} = action; + // TODO: fix that this doesn't jumpt selection if items are shifted! sorting is swapped etc + const idx = config.dataSource.getIndexOfKey(id); + if (idx !== -1) { + draft.selection = castDraft( + computeSetSelection(draft.selection, idx, addToSelection), + ); + } + break; + } case 'addRangeToSelection': { const {start, end, allowUnselect} = action; draft.selection = castDraft( @@ -241,6 +259,7 @@ export type DataTableManager = { end: number, allowUnselect?: boolean, ): void; + selectItemById(id: string | number, addToSelection?: boolean): void; clearSelection(): void; getSelectedItem(): T | undefined; getSelectedItems(): readonly T[]; @@ -261,6 +280,9 @@ export function createDataTableManager( selectItem(index: number, addToSelection = false) { dispatch({type: 'selectItem', nextIndex: index, addToSelection}); }, + selectItemById(id, addToSelection = false) { + dispatch({type: 'selectItemById', id, addToSelection}); + }, addRangeToSelection(start, end, allowUnselect = false) { dispatch({type: 'addRangeToSelection', start, end, allowUnselect}); }, diff --git a/desktop/flipper-plugin/src/ui/data-table/TableContextMenu.tsx b/desktop/flipper-plugin/src/ui/data-table/TableContextMenu.tsx index aef7284d7..914e9ae2b 100644 --- a/desktop/flipper-plugin/src/ui/data-table/TableContextMenu.tsx +++ b/desktop/flipper-plugin/src/ui/data-table/TableContextMenu.tsx @@ -11,6 +11,7 @@ import {CopyOutlined, FilterOutlined} from '@ant-design/icons'; import {Checkbox, Menu} from 'antd'; import { DataTableDispatch, + getSelectedItem, getSelectedItems, Selection, } from './DataTableManager'; @@ -21,12 +22,18 @@ import {DataSource} from '../../state/DataSource'; const {Item, SubMenu} = Menu; +function defaultOnCopyRows(items: T[]) { + return JSON.stringify(items.length > 1 ? items : items[0], null, 2); +} + export function tableContextMenuFactory( datasource: DataSource, dispatch: DataTableDispatch, selection: Selection, columns: DataTableColumn[], visibleColumns: DataTableColumn[], + onCopyRows: (rows: T[]) => string = defaultOnCopyRows, + onContextMenu?: (selection: undefined | T) => React.ReactElement, ) { const lib = tryGetFlipperLibImplementation(); if (!lib) { @@ -40,6 +47,9 @@ export function tableContextMenuFactory( return ( + {onContextMenu + ? onContextMenu(getSelectedItem(datasource, selection)) + : null} } @@ -81,9 +91,7 @@ export function tableContextMenuFactory( onClick={() => { const items = getSelectedItems(datasource, selection); if (items.length) { - lib.writeTextToClipboard( - JSON.stringify(items.length > 1 ? items : items[0], null, 2), - ); + lib.writeTextToClipboard(onCopyRows(items)); } }}> Copy row(s) @@ -94,9 +102,7 @@ export function tableContextMenuFactory( onClick={() => { const items = getSelectedItems(datasource, selection); if (items.length) { - lib.createPaste( - JSON.stringify(items.length > 1 ? items : items[0], null, 2), - ); + lib.createPaste(onCopyRows(items)); } }}> Create paste diff --git a/desktop/flipper-plugin/src/ui/data-table/TableHead.tsx b/desktop/flipper-plugin/src/ui/data-table/TableHead.tsx index 21543fdf7..ff701dfce 100644 --- a/desktop/flipper-plugin/src/ui/data-table/TableHead.tsx +++ b/desktop/flipper-plugin/src/ui/data-table/TableHead.tsx @@ -111,7 +111,11 @@ const TableHeadContainer = styled.div({ borderBottom: `1px solid ${theme.dividerColor}`, backgroundColor: theme.backgroundWash, userSelect: 'none', + whiteSpace: 'nowrap', borderLeft: `4px solid ${theme.backgroundWash}`, // space for selection, see TableRow + // hardcoded value to correct for the scrollbar in the main container. + // ideally we should measure this instead. + paddingRight: 15, }); TableHeadContainer.displayName = 'TableHead:TableHeadContainer'; diff --git a/desktop/flipper-plugin/src/ui/data-table/TableRow.tsx b/desktop/flipper-plugin/src/ui/data-table/TableRow.tsx index 202f7eb7e..b0836e7a0 100644 --- a/desktop/flipper-plugin/src/ui/data-table/TableRow.tsx +++ b/desktop/flipper-plugin/src/ui/data-table/TableRow.tsx @@ -48,6 +48,7 @@ const TableBodyRowContainer = styled.div( borderLeft: props.highlighted ? `4px solid ${theme.primaryColor}` : `4px solid transparent`, + borderBottom: `1px solid ${theme.dividerColor}`, paddingTop: 1, minHeight: DEFAULT_ROW_HEIGHT, lineHeight: `${DEFAULT_ROW_HEIGHT - 2}px`, @@ -74,7 +75,6 @@ const TableBodyColumnContainer = styled.div<{ flexGrow: props.width === undefined ? 1 : 0, overflow: 'hidden', padding: `0 ${theme.space.small}px`, - borderBottom: `1px solid ${theme.dividerColor}`, verticalAlign: 'top', // pre-wrap preserves explicit newlines and whitespace, and wraps as well when needed whiteSpace: props.multiline ? 'pre-wrap' : 'nowrap', diff --git a/desktop/flipper-plugin/src/ui/data-table/__tests__/DataTable.node.tsx b/desktop/flipper-plugin/src/ui/data-table/__tests__/DataTable.node.tsx index 38915375a..2d80db289 100644 --- a/desktop/flipper-plugin/src/ui/data-table/__tests__/DataTable.node.tsx +++ b/desktop/flipper-plugin/src/ui/data-table/__tests__/DataTable.node.tsx @@ -55,15 +55,15 @@ test('update and append', async () => { expect(elem.length).toBe(1); expect(elem[0].parentElement).toMatchInlineSnapshot(`
test DataTable
true
@@ -112,15 +112,15 @@ test('column visibility', async () => { expect(elem.length).toBe(1); expect(elem[0].parentElement).toMatchInlineSnapshot(`
test DataTable
true
@@ -137,10 +137,10 @@ test('column visibility', async () => { expect(elem.length).toBe(1); expect(elem[0].parentElement).toMatchInlineSnapshot(`
test DataTable