[Enhancement] #848 increase column width and page size #934
Conversation
Signed-off-by: kohinoor98 <[email protected]>
Signed-off-by: kohinoor98 <[email protected]>
kohinoor98
requested review from
amsiglan,
AWSHurneyt,
bowenlan-amzn,
getsaurabh02,
praveensameneni,
xluo-aws,
gaobinlong,
Hailong-am,
SuZhou-Joe,
lezzago,
sbcd90 and
eirsep
as code owners
kohinoor98
changed the title
Signed-off-by: kohinoor98 <[email protected]>
| @@ -14,6 +14,7 @@ const renderNumber = (value) => { | |||
| }; | |||
|
|
|||
| export const DEFAULT_PAGE_SIZE_OPTIONS = [5, 10, 20, 50]; | |||
| export const DEFAULT_PAGE_SIZE_OPTIONS_INDICES = [10, 20, 50, 100, 500]; | |||
There was a problem hiding this comment.
Can you add unit test for this pagination logic
| @@ -45,7 +46,7 @@ const getColumns = (props: IColumnOptions): EuiTableFieldDataColumnType<ManagedC | |||
| sortable: true, | |||
| truncateText: false, | |||
| textOnly: true, | |||
| width: "250px", | |||
| width: "320px", | |||
There was a problem hiding this comment.
Would it be possible to let user drag the width of our table?
Currently although the text is capped, I remember user can hover on it and see the name which is fine.
Adding width here is not the best because people may have very long index name 😅
There was a problem hiding this comment.
@bowenlan-amzn Based on this I will go ahead and make the switch to the Data Grid component
There was a problem hiding this comment.
Great, @kamingleung is our UX expert and thanks him for guiding here!
Signed-off-by: kohinoor98 <[email protected]>
| case "index": | ||
| return <IndexDetail {...this.props} index={rowData.index} />; |
There was a problem hiding this comment.
Loading this IndexDetail component is the cause of endless call. @kohinoor98 can you reach out to @SuZhou-Joe the writter for some guidance here.
There was a problem hiding this comment.
Yup, will do. Thanks @bowenlan-amzn
|
@kohinoor98 Would you mind posting screenshots or video capture on the pages that have been migrated to DataGrid component? Thanks. |
| @@ -37,6 +40,7 @@ import { SECURITY_EXCEPTION_PREFIX } from "../../../../../server/utils/constants | |||
| import IndicesActions from "../IndicesActions"; | |||
| import { destroyListener, EVENT_MAP, listenEvent } from "../../../../JobHandler"; | |||
| import "./index.scss"; | |||
| import IndexDetail from "../../../IndexDetail/containers/IndexDetail"; | |||
There was a problem hiding this comment.
| import IndexDetail from "../../../IndexDetail/containers/IndexDetail"; | |
| import IndexDetail from "../../containers/IndexDetail"; |
You are using wrong component here.
There was a problem hiding this comment.
The IndexDetail inside IndexDetail page is a super heavy component and should not be rendered inside a cell of data grid.
| @@ -6,6 +6,7 @@ | |||
| import { PluginInitializerContext } from "opensearch-dashboards/public"; | |||
| import { IndexManagementPlugin } from "./plugin"; | |||
| import { Action, UIAction } from "../models/interfaces"; | |||
| import "@opensearch-project/oui/dist/oui_theme_light.css"; | |||
There was a problem hiding this comment.
The theme and its css should already be imported in OSD core, I do not think we should explicitly import theme.css in a plugin.
There was a problem hiding this comment.
Yup @SuZhou-Joe. I got rid of OUI(faced issues with the library and documentation) entirely and switched to EuiDataGrid.
I am still working on the pagination part for the table. I will push the latest code with screenshots as I get some time to test for edge cases.
Thanks,
KC
Description
Issues Resolved
#848
Check List
yarn run test:jest -uBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.