-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Extracted DiscoverGrid to a package named @kbn/unified-data-table as UnifiedDataTable component #163211
Extracted DiscoverGrid to a package named @kbn/unified-data-table as UnifiedDataTable component #163211
Conversation
…ta_table_package # Conflicts: # packages/kbn-unified-data-table/src/components/data_table.scss # src/plugins/discover/public/embeddable/saved_search_embeddable.tsx
…ta_table_package # Conflicts: # src/plugins/discover/common/constants.ts # src/plugins/discover/public/application/context/context_app.tsx # src/plugins/discover/public/application/main/components/layout/discover_documents.tsx # src/plugins/discover/public/embeddable/saved_search_embeddable.tsx # src/plugins/discover/public/embeddable/saved_search_grid.tsx
cbf29ca
to
1a825a5
Compare
…-ref HEAD~1..HEAD --fix'
…ta_table_package # Conflicts: # packages/kbn-unified-data-table/src/components/data_table.scss # packages/kbn-unified-data-table/src/components/data_table.test.tsx # packages/kbn-unified-data-table/src/components/data_table.tsx # packages/kbn-unified-data-table/src/components/discover_grid_footer.test.tsx # packages/kbn-unified-data-table/src/components/discover_grid_footer.tsx # src/plugins/discover/common/constants.ts # src/plugins/discover/public/application/context/context_app_content.tsx # src/plugins/discover/public/application/main/components/layout/discover_documents.tsx # src/plugins/discover/public/embeddable/saved_search_grid.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! Thanks for taking this on. I tested a few different scenarios and everything seems to be functional. Just a couple of very minor notes below.
@@ -63,7 +65,7 @@ export const ExpandButton = ({ rowIndex, setCellProps }: EuiDataGridCellValueEle | |||
return ( | |||
<EuiToolTip content={buttonLabel} delay="long" ref={toolTipRef}> | |||
<EuiButtonIcon | |||
id={rowIndex === 0 ? DISCOVER_TOUR_STEP_ANCHOR_IDS.expandDocument : undefined} | |||
id={rowIndex === 0 ? tourStep : undefined} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not exactly sure what this is supposed to do but it seems we're not setting tourStep
anywhere, so I'm wondering if that was just missed somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in the commit 5a7067f
data: DataPublicPluginStart; | ||
fieldFormats: FieldFormatsStart; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note that we will either want to eventually extract these into some sort of wrapper for this component or add props for what necessitates these services (not for this PR).
const currentPageSize = | ||
typeof rowsPerPageState === 'number' && rowsPerPageState > 0 | ||
? rowsPerPageState | ||
: defaultRowsPerPage; | ||
: DEFAULT_ROWS_PER_PAGE; | ||
const [pagination, setPagination] = useState({ | ||
pageIndex: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason pageIndex is handled by internal state? I was surprised to see it not in the list of props. Is this a design decision wrt to browser history maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just that there was no need to handle the pagination externally in Discover so far, which doesn't mean it could be useful for other use cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, yea I'm looking to make use of this new table in Cloud security (Findings). Currently pageIndex is part of our url history, but I wonder what the standard across all of kibana is. I can't imagine deep linking to a particular page of results is all that useful, but the back/forward behavior wrt to switching pages could be something customers expect. cc @dimadavid if you had any thoughts around this, and if we have any established UX guidelines. cc @kfirpeled (is it important we keep pageIndex as part of the urlQuery?)
…ta_table_package # Conflicts: # .github/CODEOWNERS # package.json # packages/kbn-unified-data-table/src/components/data_table.tsx # packages/kbn-unified-data-table/src/components/data_table_columns.tsx # packages/kbn-unified-data-table/src/components/default_cell_actions.tsx # packages/kbn-unified-data-table/src/components/json_code_editor/__snapshots__/json_code_editor.test.tsx.snap # packages/kbn-unified-data-table/src/components/json_code_editor/json_code_editor.test.tsx # packages/kbn-unified-data-table/src/components/json_code_editor/json_code_editor.tsx # packages/kbn-unified-data-table/src/components/json_code_editor/json_code_editor_common.tsx # packages/kbn-unified-data-table/src/table_context.tsx # packages/kbn-unified-data-table/src/types.ts # packages/kbn-unified-data-table/src/utils/get_render_cell_value.tsx # src/plugins/discover/public/application/context/context_app.tsx # src/plugins/discover/public/application/context/context_app_content.tsx # src/plugins/discover/public/application/main/components/layout/discover_documents.tsx # src/plugins/discover/public/application/main/components/layout/discover_layout.tsx # src/plugins/discover/public/application/main/components/layout/discover_main_content.tsx # src/plugins/discover/public/components/discover_grid_flyout/discover_grid_flyout.tsx # src/plugins/discover/public/components/doc_table/doc_table_wrapper.tsx # src/plugins/discover/public/components/json_code_editor/__snapshots__/json_code_editor.test.tsx.snap # src/plugins/discover/public/components/json_code_editor/json_code_editor.test.tsx # src/plugins/discover/public/components/json_code_editor/json_code_editor.tsx # src/plugins/discover/public/components/json_code_editor/json_code_editor_common.tsx # src/plugins/unified_doc_viewer/public/components/doc_viewer_source/source.test.tsx # src/plugins/unified_doc_viewer/public/components/doc_viewer_source/source.tsx # src/plugins/unified_doc_viewer/public/components/doc_viewer_table/legacy/table_cell_actions.tsx # src/plugins/unified_doc_viewer/public/components/doc_viewer_table/table_cell_actions.tsx # src/plugins/unified_doc_viewer/public/components/json_code_editor/__snapshots__/json_code_editor.test.tsx.snap # src/plugins/unified_doc_viewer/public/components/json_code_editor/json_code_editor.scss # src/plugins/unified_doc_viewer/public/components/json_code_editor/json_code_editor.test.tsx # src/plugins/unified_doc_viewer/public/components/json_code_editor/json_code_editor.tsx # src/plugins/unified_doc_viewer/public/components/json_code_editor/json_code_editor_common.tsx # tsconfig.base.json # x-pack/plugins/translations/translations/fr-FR.json # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json # yarn.lock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work on this! I wanted to hold off on my review until you got a chance to address the Unified Doc Viewer conflicts, but looks like those are all solved now. The code changes look great, and I tested pretty extensively locally and didn't encounter any changes in behaviour. Thanks for all the effort on this, it's a huge help. LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this, it's super helpful!
@elasticmachine merge upstream |
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @YulNaumenko |
…UnifiedDataTable component (elastic#163211) Current PR includes the next set of changes: 1. Moved `DiscoverGrid` component to a package `@kbn/unified-data-table` and added `@elastic/kibana-data-discovery` as code owners. 2. Changed `@kbn/unified-data-table` package naming for data grid related components and methods to correspond `UnifiedDataTable` instead of `Discover`. 3. Moved hooks `useColumns` and `useRowHeightsOptions` to a package as its logic belongs to `UnifiedDataTable`. 4. Renamed `DiscoverGridContext` to `UnifiedDataTableContext`. 5. Extended `UnifiedDataTable` interface and functionality with the next customization options: - `renderDocumentView?: (displayedRows: DataTableRecord[],displayedColumns: string[]) => JSX.Element | undefined;` - callback to render DocumentView when the document is expanded - `configRowHeight?: number;` - optional value for providing configuration setting for UnifiedDataTable rows height - `showMultiFields?: boolean;` - optional value for providing configuration setting for enabling to display the complex fields in the table. Default is true. - `maxDocFieldsDisplayed?: number;` - optional value for providing configuration setting for maximum number of document fields to display in the table. Default is 50. - `externalControlColumns?: EuiDataGridControlColumn[];` - optional value for providing EuiDataGridControlColumn list of the additional leading control columns. UnifiedDataTable includes two control columns: Open Details and Select. <img width="522" alt="Screenshot 2023-08-22 at 2 26 57 PM" src="https://github.com/elastic/kibana/assets/55110838/d796b9c8-2fef-4bcc-a3c9-9f5cc6349ab9"> - `externalAdditionalControls?: React.ReactNode;` - optional value for providing the additional controls available in the UnifiedDataTable toolbar to manage it's records or state. UnifiedDataTable includes Columns, Sorting and Bulk Actions. <img width="673" alt="Screenshot 2023-08-22 at 2 40 28 PM" src="https://github.com/elastic/kibana/assets/55110838/f7ac0c87-5310-49dd-9084-1ce01ca0f366"> - `rowsPerPageOptions?: number[];` - optional list of number type values to set custom UnifiedDataTable paging options to display the records per page. - `renderCustomGridBody?: (args: EuiDataGridCustomBodyProps) => React.ReactNode;` - An optional function called to completely customize and control the rendering of EuiDataGrid's body and cell placement. <img width="1658" alt="Screenshot 2023-08-22 at 2 50 27 PM" src="https://github.com/elastic/kibana/assets/55110838/14adc18d-73af-40f5-9859-b3c708e265b1"> - `trailingControlColumns?: EuiDataGridControlColumn[];` - optional list of the `EuiDataGridControlColumn` type for setting trailing control columns standard for `EuiDataGrid`. - `visibleCellActions?: number;` - optional value for a custom number of the visible cell actions in the table <img width="497" alt="Screenshot 2023-08-22 at 2 45 49 PM" src="https://github.com/elastic/kibana/assets/55110838/57ef3ad9-7401-46bb-9b38-cc8cca2e6a24"> - `externalCustomRenderers?: Record<string,(props: EuiDataGridCellValueElementProps) => React.ReactNode>;` - an optional settings for a specified fields rendering like links. Applied only for the listed fields rendering: <img width="1121" alt="Screenshot 2023-08-22 at 2 51 07 PM" src="https://github.com/elastic/kibana/assets/55110838/77501eae-3046-4a2c-90e1-2db487c21e2c"> - `consumer` - optional string value for the name of the `UnifiedDataTable` consumer component or application. 6. Extended `UnifiedDataGrid` services with the two additional: `storage: Storage;` `data: DataPublicPluginStart; ` replaced `core: CoreStart;` with `theme: ThemeServiceStart;`, because `core` is used only to get `theme` 7. Replaced `DocumentView` property with `renderDocumentView?: (displayedRows: DataTableRecord[],displayedColumns: string[]) => JSX.Element | undefined;` callback function, which allows not to use `DiscoverGridFlyout` definition for the documents rendering. ``` /** * Document detail view component */ DocumentView?: typeof DiscoverGridFlyout; ``` 8. Removed the next properties from the data table interface, because it was used to render DiscoverGridFlyout: ``` /** * Filters applied by saved search embeddable */ filters?: Filter[]; /** * Query applied by KQL bar or text based editor */ query?: Query | AggregateQuery; /** * Saved search id used for links to single doc and surrounding docs in the flyout */ savedSearchId?: string; ``` 9. Added usage examples and interface description to README file. 10. Changed grid styles naming from `.dscDiscoverGrid*` to `.udtDataTable*` 11. Migrated discover plugin to use `UnifiedDataTable` instead of `DiscoverGrid` Extra changes were needed to avoid the circular dependancies: - moved `DocViewFilterFn` and `FieldMapping` from discover plugin to `packages/kbn-discover-utils/src/types.ts` - added own `export type SortOrder = [string, string];` to avoid deps for saved-search plugin --------- Co-authored-by: kibanamachine <[email protected]>
Summary
Current PR includes the next set of changes:
Moved
DiscoverGrid
component to a package@kbn/unified-data-table
and added@elastic/kibana-data-discovery
as code owners.Changed
@kbn/unified-data-table
package naming for data grid related components and methods to correspondUnifiedDataTable
instead ofDiscover
.Moved hooks
useColumns
anduseRowHeightsOptions
to a package as its logic belongs toUnifiedDataTable
.Renamed
DiscoverGridContext
toUnifiedDataTableContext
.Extended
UnifiedDataTable
interface and functionality with the next customization options:renderDocumentView?: (displayedRows: DataTableRecord[],displayedColumns: string[]) => JSX.Element | undefined;
- callback to render DocumentView when the document is expandedconfigRowHeight?: number;
- optional value for providing configuration setting for UnifiedDataTable rows heightshowMultiFields?: boolean;
- optional value for providing configuration setting for enabling to display the complex fields in the table. Default is true.maxDocFieldsDisplayed?: number;
- optional value for providing configuration setting for maximum number of document fields to display in the table. Default is 50.externalControlColumns?: EuiDataGridControlColumn[];
- optional value for providing EuiDataGridControlColumn list of the additional leading control columns. UnifiedDataTable includes two control columns: Open Details and Select.externalAdditionalControls?: React.ReactNode;
- optional value for providing the additional controls available in the UnifiedDataTable toolbar to manage it's records or state. UnifiedDataTable includes Columns, Sorting and Bulk Actions.rowsPerPageOptions?: number[];
- optional list of number type values to set custom UnifiedDataTable paging options to display the records per page.renderCustomGridBody?: (args: EuiDataGridCustomBodyProps) => React.ReactNode;
- An optional function called to completely customize and control the rendering of EuiDataGrid's body and cell placement.trailingControlColumns?: EuiDataGridControlColumn[];
- optional list of theEuiDataGridControlColumn
type for setting trailing control columns standard forEuiDataGrid
.visibleCellActions?: number;
- optional value for a custom number of the visible cell actions in the tableexternalCustomRenderers?: Record<string,(props: EuiDataGridCellValueElementProps) => React.ReactNode>;
- an optional settings for a specified fields rendering like links. Applied only for the listed fields rendering:consumer
- optional string value for the name of theUnifiedDataTable
consumer component or application.UnifiedDataGrid
services with the two additional:storage: Storage;
data: DataPublicPluginStart;
replaced
core: CoreStart;
withtheme: ThemeServiceStart;
, becausecore
is used only to gettheme
DocumentView
property withrenderDocumentView?: (displayedRows: DataTableRecord[],displayedColumns: string[]) => JSX.Element | undefined;
callback function, which allows not to useDiscoverGridFlyout
definition for the documents rendering..dscDiscoverGrid*
to.udtDataTable*
UnifiedDataTable
instead ofDiscoverGrid
Extra changes were needed to avoid the circular dependancies:
DocViewFilterFn
andFieldMapping
from discover plugin topackages/kbn-discover-utils/src/types.ts
export type SortOrder = [string, string];
to avoid deps for saved-search plugin