[Research] OUI usage audit in the expressions plugin

[BSFishy]

Audit

The expressions plugin has 2 Sass files: index.scss and _expression_renderer.scss. Index just includes the other one, which defines a few classes:

OpenSearch-Dashboards/src/plugins/expressions/public/_expression_renderer.scss

Lines 1 to 25 in 61ea841

	.expExpressionRenderer {
	position: relative;
	display: flex;
	align-items: center;
	justify-content: center;
	width: 100%;
	height: 100%;
	}
	.expExpressionRenderer__expression {
	display: flex;
	flex-direction: column;
	width: 100%;
	height: 100%;
	overflow: auto;
	position: relative;
	flex: 1 1 100%;
	}
	.expExpressionRenderer-isEmpty,
	.expExpressionRenderer-hasError {
	.expExpressionRenderer__expression {
	display: none;
	}
	}

Related React code:

OpenSearch-Dashboards/src/plugins/expressions/public/react_expression_renderer.tsx

Lines 187 to 212 in 61ea841

	const classes = classNames('expExpressionRenderer', className, {
	'expExpressionRenderer-isEmpty': state.isEmpty,
	'expExpressionRenderer-hasError': !!state.error,
	});
	const expressionStyles: React.CSSProperties = {};
	if (padding) {
	expressionStyles.padding = theme.paddingSizes[padding];
	}
	return (
	<div {...dataAttrs} className={classes}>
	{state.isEmpty && <EuiLoadingChart mono size="l" />}
	{state.isLoading && <EuiProgress size="xs" color="accent" position="absolute" />}
	{!state.isLoading &&
	state.error &&
	renderError &&
	renderError(state.error.message, state.error)}
	<div
	className="expExpressionRenderer__expression"
	style={expressionStyles}
	ref={mountpoint}
	/>
	</div>
	);

For the most part, these can be replaced with EuiPanel, EuiFlexGroup, and EuiFlexItem. However, there are a few misses from OUI:

1. OUI doesn't provide any way to do display: none. It is important we apply this value because we need to get a ref to the element this is applied to while data is loading.
2. OUI doesn't provide anything for overflow: auto. Not sure how necessary this is, but it might be nice to have parity with the older version. I think this might be useful when the parent is size constricting, so it adds a scrollbar.
3. OUI doesn't allow editing flex-shrink or flex-basis on EuiFlexItem. Again, not sure how important this is, but it would be nice to have parity.

Conclusion

We should follow these action items:

1. Create OUI utility class oui-displayNone to apply display: none
2. Create OUI utility class oui-overflowAuto to apply overflow: auto
3. Allow EuiFlexItem to modify flex-shrink and flex-basis

Once those changes are made to OUI, the React code can be modified to use OUI:

  const classes = classNames('oui-overflowAuto', {
    'oui-displayNone': !state.isEmpty && !state.error,
  });

  return (
    <EuiFlexGroup {...dataAttrs} alignItems="center" justifyContent="center" gutterSize="none" className={className}>
      {state.isEmpty && <EuiLoadingChart mono size="l" />}
      {state.isLoading && <EuiProgress size="xs" color="accent" position="absolute" />}
      {!state.isLoading &&
        state.error &&
        renderError &&
        renderError(state.error.message, state.error)}
      <EuiFlexItem grow={1}>
        <EuiPanel
          paddingSize={padding}
          hasShadow={false}
          hasBorder={false}
          color="transparent"
          className={classes}
          panelRef={mountpoint}
        />
      </EuiFlexItem>
    </EuiFlexGroup>
  );

[joshuarrrr]

It is important we apply this value because we need to get a ref to the element this is applied to while data is loading.

Can you explain more about why this is necessary? In your proposed solution, there's still a lot of rendering logic going on for what is a very common pattern. The panel has some initial empty state, followed by a loading state, and then a success or error state. I'm not sure I understand why we'd need the success state component (EuiPanel) to be rendered but hidden in any of the other states. Is it just to fill up the space?

OUI doesn't provide anything for overflow: auto. Not sure how necessary this is, but it might be nice to have parity with the older version. I think this might be useful when the parent is size constricting, so it adds a scrollbar.

I think a fixed-size container with scrollable contents is definitely a use-case we need to solve for in OUI, and my gut would lean toward making OuiPanel more customizable. In your proposed solution, I'm not convinced we need OuiFlexGroup or OuiFlexItem at all. Why can't LoadingChart, Progress, and Error states be children/contents of the panel?

For the benefit of UX, can you provide screenshots that break down the states and phases of this expression component, so we can have a better perspective on what's happening with the DOM at each point.

[joshuarrrr]

I did a little research and found that, while the expresions plugin is used heavily (by nearly every visualization type, the ReactExpressionRendereris only used byvis_builder`. You can see an example on playground by creating a new vis_builder visualization: https://playground.opensearch.org/app/vis-builder/#/?_q=(filters:!(),query:(language:kuery,query:''))&_a=(metadata:(editor:(errors:(),state:clean)),style:(addLegend:!t,addTooltip:!t,legendPosition:right,type:histogram),ui:(),visualization:(activeVisualization:(aggConfigParams:!(),name:histogram),indexPattern:ff959d40-b880-11e8-a6d9-e546fe2bba5f,searchField:''))&_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:now))