[Proposal] component configuration pattern

[chandlerprall]

This came up during a chat about how to allow a cell's popover to completely dismiss the buttons we render for the cell actions (#5132). I don't know what the pattern is called, but the elastic charts library uses this for providing configurations, as do a number of other libraries.

While considering the existing setCellProps API, I thought it could be cleaned up by allowing the configuration be set by a component rather than a function call. I've added a new CellProps component API in a branch and refactored that example to show this pattern: https://github.com/constancecchen/eui/compare/datagrid/5310/cell-popovers...chandlerprall:datagrid/cell-configs?expand=1 (NB: this relies on changes in @constancecchen's cell-popovers branch)

datagrid.js

      // Apply custom background color to cells in the `amount` columns
      let cellProps = undefined;
      if (columnId === 'amount') {
        const numeric = parseFloat(content.match(/\d+\.\d+/)[0], 10);
        cellProps = (
          <CellProps
            props={{
              style: {
                backgroundColor: `rgba(0, 255, 0, ${numeric * 0.0002})`,
              },
            }}
          />
        );
      }

      return (
        <>
          {content}
          {cellProps}
        </>
      );

This same pattern could be used for the expansion popover to be told not to render the default action buttons.

[chandlerprall]

Another place this could be used, though likely a terrible, terrible idea: this could be used by individual cells to provide row props (see #5555). Cells could provide any prop they want to see on the row, and those would be merged together at a row unit. This idea has other complications such as there is no row unit but hey, we're just thinking out loud here.

[snide]

It's a terrible idea from a uniformity sense, but it's a feature that I think there is likely a real use case for. Is this map data? Do this...etc. It makes it so you don't have to have a giant function to handle the logic of what happens once it's open.

[cee-chen]

I think I'd rather see us implement a new top level renderRowElement prop to go along with renderCellValue rather than that approach 🙈

[cee-chen]

Speaking of though, this does make me wonder about the consistency of our various component APIs. For example: why not use config components for all of data grid to replace renderCellValue, renderFooterCellValue, columns.display? Something like

<EuiDataGrid
  // ... required props
>
  ({ EuiDataGridCell, EuiDataGridRow, EuiDataGridHeaderCell, EuiDataGridFooterCell }) => (
    <EuiDataGridRow {...customProps} />
    <EuiDataGridHeaderCell {...customProps}>{customContent}</EuiDataGridHeaderCell>
    <EuiDataGridCell {...customProps}>{customContent}</EuiDataGridCell>
    <EuiDataGridFooterCell {...customProps}>{customContent}</EuiDataGridCell>
  )
</EuiDataGrid>

[chandlerprall]

I'm going to look through components for other places this pattern could be useful; if they don't exist it's unlikely the right choice to introduce it here.

[chandlerprall]

I don't see a strong reason to bring this pattern in, especially with the hooks thoughts below. Page templates seem like the best location, but that could be changed in other ways too - or left alone. I'm gonna accept the hook answer below to close this out.

[cee-chen]

One quick comment on

eui/src/components/datagrid/body/data_grid_cell.tsx

Line 693 in fc51609

<CellPropsContext.Provider value={{ setCellProps: this.setCellProps }}>

How will this work in the future when we add more config props for, e.g. the cell popover footer, or perhaps allowing cells to set/configure isExpandable on the per-cell level instead of per-column? I strongly don't think we should add a new context for each configuration component, so should we instead prefer to call the context something more generic like ConfigComponents.Provider?

Or, with future configuration in mind, are we simply adding all possible configurations to a cell as <CellProps />? e.g., <CellProps hideCellActions={true} isExpandable={true} />?

Or alternatively: why not bypass context completely? If we declare CellProps as a class method/function within EuiDataGridCell, we can still pass it as a renderCellValue arg and it has access to all the internal cell states it needs. ...Although, thinking more on that approach, I'm not sure how well that would scale (since the data grid cell class would become more bloated the more components we potentially add).

[chandlerprall]

CellWrapper component

(idea from a conversation with @flash1293 )

Instead of providing a CellProps component to the render function, we could pass a CellWrapper component:

const RenderCellValue = ({ CellWrapper }) => (
  <CellWrapper style={}>
     {cellContents} 
  </CellWrapper>
);

this CellWrapper could either be 1. effectively the same thing as the proposed CellProps, or 2. could be the internal EuiDataGridCellContent, inverting the control but avoids a render->useEffect->render loop just to provide styles.

[flash1293]

I would prefer the hook idea from below in general but just one note here

inverting the control but avoids a render->useEffect->render loop just to provide styles

Makes it sound a bit like it's a bad thing to invert to control flow, I actually like it because it gets rid of implicit dependencies using context which I think is a good property.

[chandlerprall]

useCellProps hook

(idea from a conversation with @flash1293)

Instead of providing a convenience component, a more common pattern is a convenience hook

const RenderCellValue = ({ useCellProps }) => {
  useCellProps({ style });
  return <div>... content ...</div>;
};

[cee-chen]

I'll admit I personally bias towards fns/hooks over components in terms of developer experience, especially when nothing is actually being rendered and renderCellValue is already essentially a functional component.