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.

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

[Fix] Datagrid Custom Body Render general fixes #8028

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

logeekal
Copy link

@logeekal logeekal commented Sep 18, 2024

Summary

This PR tries to provide more control to the users of DataGrid when they pass renderCustomBody prop. Below are the changes done.

Changes

  • Users can not provide style to Cell as a part of renderCustomBody component.
  • Currentlly, EUI treats the extra row same as other cells of the table, giving it a height same as other cells. However, that cell acting as extra row should be given a height of auto or undefined so that it can take up space occupied by its content
  • headerRow and footerRow are now passed to renderCustomBody component for the user of data grid to render. This was done so that if users are rendering rows in a custom manner, those customization can also be applied to header and footer rows. For example, I was trying to use a virtualized table component where there was option to scroll the table horizontally, since headerRow was already rendered by EUI, I did not have option to contain it within a section same as table and hence scrolling was different in headerRow and the actual table.

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
    • If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist
    • If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 18, 2024

💔 Build Failed

Failed CI Steps

History

Comment on lines +614 to +618
this.props.columnId === 'timeline-event-detail-row'
? undefined
: style?.height, // row height, can be undefined
top: style?.top ? 0 : undefined, // The cell's row will handle top positioning
width, // column width, can be undefined
width: width === 0 ? '100%' : width, // column width, can be undefined
Copy link
Member

@cee-chen cee-chen Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes feel a little spicy to me, I'm not a huge fan of them. If they're for the custom row detail, don't forget that you can override the cellProps.style via setCellProps within your renderCellValue component, e.g.

<EuiDataGrid
  renderCustomGridBody={...}
  renderCellValue={({ columnId, setCellProps }) => {
    useEffect(() => {
      if (columnId === 'timeline-event-detail-row') {
        setCellProps({ style: { width: '100%', height: undefined } })
      }
    }, [columnId, setCellProps]);

    return ...
  }}
/>

Copy link
Author

@logeekal logeekal Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @cee-chen,

Thanks for the comment.

Below change is gone and is not not needed. It was to debug something. I left it here by mistake.

        this.props.columnId === 'timeline-event-detail-row'
          ? undefined
          : style?.height, // row height, can be undefined

About this change :

      width: width === 0 ? '100%' : width, // column width, can be undefined

Problem

I do not disagree but i have not been able to find the better alternative. I have setup an example for you here. This is almost exactly what i am implementing in Security Solution : https://codesandbox.io/p/sandbox/custom-body-render-react-window-vm9m6k?file=%2Fdemo.tsx%3A119%2C3-119%2C15

Problem is that when setCellProps is run, it does not actually updates the Cell component which is really important to measure the size of cell component.

  • This is because it only updates the css.
  • That css is actually overriden by the these recaculateAutoHeight and recalculateLineHeight functions.
  • Now i was thinking what could be the best way to override or calculate height settings for Extra Row. Long term solution could be accept extra row component and treat it separately instead of applying settings of all the cells to that cell. But for now, i thought that do combine above change + this change to get around this problem.

Demo

Now in our case of Custom Row, the width is initially 0 as passed to EuiDataGrid in trailingColumns config ( which also feels incorrect imo ). This creates a problem when the row is actually rendered, it starts with a width of 0 which is measured as soon as it is rendered and additionally height is autoHeight which is actually equal to the height of all other cells. But that Custom Row cell is not like other cells so it should not be treated as such.

To see that in demo, go to the codesandbox and search for Row-0 in console messages. It will show the transition in the dimensions of the DataGrid Rows. You will observe that event setCellProps has been triggered, the Cell with custom row is only updated with initial dimension.

That is why, I updated the logic to have correct initial dimension as 0 seemed incorrect. Let me know if there is a better way I can go around it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants