Open BSFishy opened 1 year ago
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.
I did a little research and found that, while the expresions
plugin is used heavily (by nearly every visualization type, the
ReactExpressionRendereris only used by
vis_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))
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:https://github.com/opensearch-project/OpenSearch-Dashboards/blob/61ea841e4eab621d86aed9274c95b1bcc2f66c55/src/plugins/expressions/public/_expression_renderer.scss#L1-L25
Related React code:
https://github.com/opensearch-project/OpenSearch-Dashboards/blob/61ea841e4eab621d86aed9274c95b1bcc2f66c55/src/plugins/expressions/public/react_expression_renderer.tsx#L187-L212
For the most part, these can be replaced with
EuiPanel
,EuiFlexGroup
, andEuiFlexItem
. However, there are a few misses from OUI: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.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.flex-shrink
orflex-basis
onEuiFlexItem
. Again, not sure how important this is, but it would be nice to have parity.Conclusion
We should follow these action items:
oui-displayNone
to applydisplay: none
oui-overflowAuto
to applyoverflow: auto
EuiFlexItem
to modifyflex-shrink
andflex-basis
Once those changes are made to OUI, the React code can be modified to use OUI: