[Table Visualization] Replace div containers with OuiFlex components

[curq]

Description

Replaces the usage of div container with OuiFlex components for better OuiCompliance. Reduced custom styling and removed a custom class.

Explanation why margin-bottom and padding where removed:

1. margin-bottom: $euiSizeL; is replaced by gutterSize prop (which adds negative margin to add distance between flex items, default value 'l' (-12px)). So there is no longer need to add margin-bottom.
2. padding: $euiSizeS; was also somewhat replaced by gutterSize.

---

Also, it looks like switching to Flex components changed the column view.
before:

after:

I think the after view is a better option from visual perspective so there is no need to try keeping before view. What are you thoughts?

---

Another important change is that in row view the tables are now distributed equally through the entire table container instead of taking all space that it needs. Here how it was before:
5 items per page

30 items per page

This is how it is now:
5 items per page

30 items per page

It only happens if we change

OpenSearch-Dashboards/src/plugins/vis_type_table/public/components/table_vis_component_group.tsx

Lines 25 to 34 in 55b293a

	{tableGroups.map(({ table, title }) => (
	<div key={title} className="visTable__group">
	<TableVisComponent
	title={title}
	table={table}
	visConfig={visConfig}
	event={event}
	uiState={uiState}
	/>
	</div>

to OuiFlexItem
I tried investigating deeper, but I didn't find the reason for such behavior change. Maybe it is related how OuiDataGrid is implemented?
I'm not sure which behavior is expected, but I think the 'after' one is more user friendly?

Issues Resolved

Solves #4255

Screenshot

To provide clear view how looks changed here are before and after screenshots for both column and row view

Before

row view:

column view:

After

row view:

column view:

Testing the changes

Use split tables option and then check both column and row views, it is also good to check how it behaves with large number of rows.

Check List

• All tests pass
  • yarn test:jest
  • yarn test:jest_integration
  • yarn test:ftr
• New functionality includes testing.
• New functionality has been documented.
• Update CHANGELOG.md
• Commits are signed per the DCO using --signoff

[codecov]

Codecov Report

Merging #4272 (bbf5492) into main (f8ad83f) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4272      +/-   ##
==========================================
- Coverage   66.16%   66.14%   -0.03%     
==========================================
  Files        3314     3314              
  Lines       63911    63912       +1     
  Branches     9970     9972       +2     
==========================================
- Hits        42288    42274      -14     
- Misses      19180    19192      +12     
- Partials     2443     2446       +3     

Flag	Coverage Δ
Linux_1	34.74% <100.00%> (+<0.01%)	⬆️
Linux_2	55.04% <ø> (ø)
Linux_3	43.17% <ø> (-0.01%)	⬇️
Linux_4	35.10% <ø> (ø)
Windows_1	?
Windows_2	55.00% <ø> (ø)
Windows_3	?
Windows_4	35.10% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...pe_table/public/components/table_vis_component.tsx	100.00% <ø> (ø)
...vis_type_table/public/components/table_vis_app.tsx	100.00% <100.00%> (ø)
...le/public/components/table_vis_component_group.tsx	100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

[joshuarrrr]

I think the after view is a better option from visual perspective so there is no need to try keeping before view. What are you thoughts?

I agree that it seems better. But when testing and validating updates to visualizations, it's important to also see how the behavior changes when the visualization is embedded in a dashboard (as well as on the visualization page, as the screenshots here cover).

Another important change is that in row view the tables are now distributed equally through the entire table container instead of taking all space that it needs.

I also like the after behavior, (pending validation in dashboard mode with small panels). But in the Screenshot -> After -> row view screenshot, the behavior looks a little different - we may need to adjust so that pagination controls always appear directly beneath the last row, not all the way at the bottom of a stretched flex item.

I'll pull this PR down and do some additional validation and screenshots.

[joshuarrrr]

@curq First, thanks for the excellent and comprehensive PR description! It really helps making the review process more efficient. The code changes look great, but I want to spend a bit more time playing with the table in both visualization and dashboard views.

[joshuarrrr]

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.

[curq]

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.

[joshuarrrr]

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

[curq]

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.

[ananzh]

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.

[ananzh]

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

[joshuarrrr]

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

[joshuarrrr]

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

[curq]

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.

[curq]

@joshuarrrr, I tried removing rest of the styles and it looks like layout didn't change at all. I'm still not sure if it safe to remove them, as before changing to Flex components removing these styles did break the layout. As I understand the OuiFlex and OuiFlexGroup do apply shrink and basis by default, although I'm not sure if it the exact values as in the current custom stylings. So, I have suspicion that FlexItem and FlexGroup are already handling our layout. But like I said, we need to carefully look through the layout.

[ananzh]

@curq I like the change. But I see a regression after quick check.

Before

After

See the spaces are missing on the left side of table. This is just table with a large time range. No split. We need to make sure the change won't have any regression issues. Could you check if you see this issue?

[curq]

@ananzh Thanks for the catch! The issue with missing space was due to TableVisComponent (used when only single table is rendered) was wrapped in </> empty tag, instead of FlexItem like in TableVisComponentGroup, so the property gutterSize of FlexGroup was not applying to it, resulting in the lack of spacing. I fixed it and now it should be fine. Could you please take a look?

[ananzh]

@ananzh Thanks for the catch! The issue with missing space was due to TableVisComponent (used when only single table is rendered) was wrapped in </> empty tag, instead of FlexItem like in TableVisComponentGroup, so the property gutterSize of FlexGroup was not applying to it, resulting in the lack of spacing. I fixed it and now it should be fine. Could you please take a look?

Nice!! Thank you~

[joshuarrrr]

@curq I'm not sure all the functional tests are completely updated? It looks like the class is still present here: https://github.com/opensearch-project/opensearch-dashboards-functional-test/blob/7f8a7d799abe560549759fa440b0184a3238b61b/cypress/integration/core-opensearch-dashboards/opensearch-dashboards/apps/vis_type_table/split.spec.js#L314

Can you try running the cypress tests against this branch again? The failure we're seeing is actually looking for the visTable__group string.

To run the cypress tests against your local OpenSearch Dashboards, start it with yarn start --no-base-path (to use port 5601 directly), and make sure both 5601 and 9200 ports are accessible, then run yarn cypress:open from the functional test repo, and you can choose a particular spec to run and debug.

[curq]

@joshuarrrr Thanks for the feedback! You're right it looks like there is single usage of visTable__groupInColumns left at the file you mentioned. Test should be fine after we remove it. I rechecked the codebase and it is the only usage of this class left.

I made a quick fix PR. Could check it please? opensearch-project/opensearch-dashboards-functional-test#785

[curq]

Can you try running the cypress tests against this branch again? The failure we're seeing is actually looking for the visTable__group string.

To run the cypress tests against your local OpenSearch Dashboards, start it with yarn start --no-base-path (to use port 5601 directly), and make sure both 5601 and 9200 ports are accessible, then run yarn cypress:open from the functional test repo, and you can choose a particular spec to run and debug.

@joshuarrrr I didn't manage to run these in my previous fix so I wasn't able to fully check solution (I use docker on mac m1). I checked the CI and noticed that it still fails.

Today I made it work so after looking through the errors, it looks like the line .find('[class="visTable__group"]') https://github.com/opensearch-project/opensearch-dashboards-functional-test/blob/7f8a7d799abe560549759fa440b0184a3238b61b/cypress/integration/core-opensearch-dashboards/opensearch-dashboards/apps/vis_builder/vis_types/table.spec.js#L91
Was causing the issue, because it matches elements with exactly single class visTable__group, but because now the container div also got euiFlexItem class, it failed to match.

I made another PR that should finally make the tests pass. I checked it on my local dashboards instance and this error is indeed fixed. In the given test update PR I provided screenshot of successful test pass. There were other tests error but I think they were unrelated to this PR.

[joshuarrrr]

all tests pass!