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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dd9c8af
replace div containers with OuiFlex
curq Jun 9, 2023
167c35a
Update test to not include removed class
curq Jun 9, 2023
c213dca
Update Changelog
curq Jun 9, 2023
a5e312e
wrap table in FlexItem
curq Jun 12, 2023
8b5ceb5
Merge branch 'main' into table-replace-div
curq Jun 12, 2023
254affd
Merge branch 'main' into table-replace-div
curq Jun 26, 2023
be0c7d9
Merge branch 'main' into table-replace-div
joshuarrrr Jul 13, 2023
dac7dbe
Merge branch 'main' into table-replace-div
joshuarrrr Jul 19, 2023
cc37ce8
Merge branch 'main' into table-replace-div
curq Jul 22, 2023
bbf5492
Merge branch 'main' into table-replace-div
joshuarrrr Jul 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- 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))
- Replace the use of `bluebird` in `saved_objects` plugin ([#4026](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4026))
- [Table Visualization] Replace div containers with OuiFlex components ([#4272](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4272))

### 🔩 Tests

Expand Down
12 changes: 0 additions & 12 deletions src/plugins/vis_type_table/public/components/table_vis_app.scss
View file
Open in desktop
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 •
edited
Loading

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;
}
2 changes: 1 addition & 1 deletion src/plugins/vis_type_table/public/components/table_vis_app.test.tsx
View file
Open in desktop
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?

OpenSearch-Dashboards/src/plugins/vis_type_table/public/components/table_vis_app.test.tsx

Lines 82 to 98 in 55b293a

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
View file
Open in desktop
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
6 changes: 3 additions & 3 deletions src/plugins/vis_type_table/public/components/table_vis_component_group.tsx
View file
Open in desktop
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