Skip to content
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

[Table Visualization] Replace div containers with OuiFlex components #4272

Merged
merged 10 commits into from
Jul 26, 2023
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Fix EUI/OUI type errors ([#3798](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3798))
- Remove unused Sass in `tile_map` plugin ([#4110](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4110))
- [Table Visualization] Remove custom styling for text-align:center in favor of OUI utility class. ([#4164](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4164))
- [Table Visualization] Replace div containers with OuiFlex components ([#4272](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4272))
- Migrate from legacy elasticsearch client to opensearch-js client in `osd-opensearch-archiver` package([#4142](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4142))
- Replace the use of `bluebird` in `saved_objects` plugin ([#4026](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4026))
- [Maps Legacy] Removed KUI usage in `maps_legacy` plugin([#3998](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3998))
Expand Down
12 changes: 0 additions & 12 deletions src/plugins/vis_type_table/public/components/table_vis_app.scss
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
// Container for the Table Visualization component
.visTable {
display: flex;
flex-direction: column;
flex: 1 0 0;
overflow: auto;
Comment on lines 3 to 4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're so close - any other OUI options to get rid of these styles? The overflow could be handled with a utility class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could wait for this one to finish opensearch-project/oui#776, and this one for utility ( we don't have option for overflow:auto right now ) opensearch-project/oui#774.


Expand All @@ -10,15 +8,5 @@

// Group container for table visualization components
.visTable__group {
padding: $euiSizeS;
margin-bottom: $euiSizeL;
display: flex;
flex-direction: column;
flex: 0 0 auto;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any ideas about how we can remove this final style and scrap the class altogether? Why is this necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have 2 options right now.

  1. If Add props for shrink and basis for OuiFlexItem oui#776 will be accepted and PR made we could just move it to component property.
  2. I tried to remove them and noticed that this styling has no effect on layout at all. I think it needs more testing and looking around so we can make sure that it really can be removed.

Copy link
Member

@ananzh ananzh Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, @curq, could we do <EuiFlexItem grow={false}> here? I think the grow prop of EuiFlexItem behaves similarly to the 'flex-grow' CSS property. So by default, EuiFlexItem components will grow and shrink as needed (i.e., flex: 1 1 auto). If you want to mimic the behavior of flex: 0 0 auto;, you should set the 'grow' property of the EuiFlexItem to false...

Not a blocker for this PR. But if you want to test it out, I think it will make it more clean and I could help to approve again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it. Seems <EuiFlexItem grow={false}> not work very well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@curq Let's just make a follow-up issue for this and we can tackle it after this PR.

}

// Modifier for visTables__group when displayed in columns
.visTable__groupInColumns {
flex-direction: row;
align-items: flex-start;
}
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ describe('TableVisApp', () => {
handlers={handlersMock}
/>
);
expect(container.outerHTML.includes('visTable visTable__groupInColumns')).toBe(true);
expect(container.outerHTML.includes('visTable')).toBe(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not required, but see if you can think of a good way in the unit tests to validate the row vs column behavior you've made conditional in L46-47.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we can remove it, right? Also, should this one be also removed?

it('should render TableVisComponentGroup component if split direction is row', () => {
const visDataMock = {
tableGroups: [],
direction: 'row',
} as TableVisData;
const { container, getByTestId } = render(
<TableVisApp
services={serviceMock}
visData={visDataMock}
visConfig={visConfigMock}
handlers={handlersMock}
/>
);
expect(container.outerHTML.includes('visTable')).toBe(true);
expect(getByTestId('TableVisComponentGroup')).toBeInTheDocument();
});
});

I will try to think about how we can test it, but for now it looks like after we converted to OUI it is not required as direction is handled by OUI components now.

expect(getByTestId('TableVisComponentGroup')).toBeInTheDocument();
});

Expand Down
16 changes: 8 additions & 8 deletions src/plugins/vis_type_table/public/components/table_vis_app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@

import './table_vis_app.scss';
import React, { useEffect } from 'react';
import classNames from 'classnames';
import { CoreStart } from 'opensearch-dashboards/public';
import { I18nProvider } from '@osd/i18n/react';
import { IInterpreterRenderHandlers } from 'src/plugins/expressions';
import { EuiFlexGroup } from '@elastic/eui';
import { PersistedState } from '../../../visualizations/public';
import { OpenSearchDashboardsContextProvider } from '../../../opensearch_dashboards_react/public';
import { TableVisData } from '../table_vis_response_handler';
Expand All @@ -35,17 +35,17 @@ export const TableVisApp = ({
handlers.done();
}, [handlers]);

const className = classNames('visTable', {
// eslint-disable-next-line @typescript-eslint/naming-convention
visTable__groupInColumns: direction === 'column',
});

const tableUiState: TableUiState = getTableUIState(handlers.uiState as PersistedState);

return (
<I18nProvider>
<OpenSearchDashboardsContextProvider services={services}>
<div className={className} data-test-subj="visTable">
<EuiFlexGroup
className="visTable"
data-test-subj="visTable"
direction={direction === 'column' ? 'row' : 'column'}
alignItems={direction === 'column' ? 'flexStart' : 'stretch'}
>
{table ? (
<TableVisComponent
table={table}
Expand All @@ -61,7 +61,7 @@ export const TableVisApp = ({
uiState={tableUiState}
/>
)}
</div>
</EuiFlexGroup>
</OpenSearchDashboardsContextProvider>
</I18nProvider>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,13 @@

import React, { useCallback, useMemo } from 'react';
import { orderBy } from 'lodash';
import { EuiDataGridProps, EuiDataGrid, EuiDataGridSorting, EuiTitle } from '@elastic/eui';
import {
EuiDataGridProps,
EuiDataGrid,
EuiDataGridSorting,
EuiTitle,
EuiFlexItem,
} from '@elastic/eui';

import { IInterpreterRenderHandlers } from 'src/plugins/expressions';
import { FormattedTableContext } from '../table_vis_response_handler';
Expand Down Expand Up @@ -102,7 +108,7 @@ export const TableVisComponent = ({
: undefined;

return (
<>
<EuiFlexItem>
{title && (
<EuiTitle size="xs" className="eui-textCenter">
<h3>{title}</h3>
Expand Down Expand Up @@ -140,6 +146,6 @@ export const TableVisComponent = ({
),
}}
/>
</>
</EuiFlexItem>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
*/

import React, { memo } from 'react';

import { IInterpreterRenderHandlers } from 'src/plugins/expressions';
import { EuiFlexItem } from '@elastic/eui';
import { TableGroup } from '../table_vis_response_handler';
import { TableVisConfig } from '../types';
import { TableVisComponent } from './table_vis_component';
Expand All @@ -23,15 +23,15 @@ export const TableVisComponentGroup = memo(
return (
<>
{tableGroups.map(({ table, title }) => (
<div key={title} className="visTable__group">
<EuiFlexItem key={title} className="visTable__group">
<TableVisComponent
title={title}
table={table}
visConfig={visConfig}
event={event}
uiState={uiState}
/>
</div>
</EuiFlexItem>
))}
</>
);
Expand Down