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

[RAM] Use EuiDataGrid for the rules list #124428

Closed
wants to merge 14 commits into from

Conversation

Zacqary
Copy link
Contributor

@Zacqary Zacqary commented Feb 2, 2022

Summary

Related to #123860

This is a feature-flagged proof of concept to use the EuiDataGrid in place of EuiBasicTable for the Rules and Connectors list.

Screen Shot 2022-02-09 at 4 09 12 PM

Usage

In kibana.dev.yml, add:

xpack.trigger_actions_ui.enableExperimental:
  - rulesListDatagrid

Known Issues

  • No pagination
  • Can only sort by one field
  • Issues with too many re-renders mean that moving columns left and right doesn't always work correctly, and column resizing might fail occasionally

@Zacqary Zacqary added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Alerting/RulesManagement Issues related to the Rules Management UX v8.2.0 labels Feb 2, 2022
@Zacqary Zacqary marked this pull request as ready for review February 9, 2022 22:11
@Zacqary Zacqary requested review from a team as code owners February 9, 2022 22:11
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

This looks good and just a tiny css nit.

Do we have a doc somewhere with a write-up for the rationale for this update? I think the question is coming up more and more about BasicTable vs DataGrid, so looking for additional reasons for the switch.

@@ -16,6 +17,14 @@
}
}

.euiDataGridHeaderCell {
font-size: 12px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit, especially knowing this is a POC, but just mentioning the pixel value. Is this necessary? Perhaps down the road we can determine how we want the various cells to render and apply font sizing there as opposed to overwriting the eui class?

@sophiec20
Copy link
Contributor

@mdefazio We were hoping the design team could already provide the guidance on BasicTable vs DataGrid. One question that came up in discussions was, is DataGrid seen as the table for the future and does it have continued investment in it?

@ryankeairns
Copy link
Contributor

@mdefazio We were hoping the design team could already provide the guidance on BasicTable vs DataGrid. One question that came up in discussions was, is DataGrid seen as the table for the future and does it have continued investment in it?

EUI provides guidance on the use of each: Tables and Data grids

I'll highlight one snippet from atop the Data grids page:

EuiDataGrid is for displaying large amounts of tabular data. It is a better choice over EUI tables when there are many columns, the data in those columns is fairly uniform, and when schemas and sorting are important for comparison. Although it is similar to traditional spreedsheet software, EuiDataGrid's current strengths are in rendering rather than creating content.

Certainly there is overlap in the general layout, but they do skew towards different cases, as described. The plan is not to replace the basic table component and both are actively enhanced and supported. As for what is needed in this particular case, a more thorough understanding of those aspects - volume, number of columns, sorting requirements, etc. - would help us better determine the best fit.

@gmmorris
Copy link
Contributor

@elasticmachine merge upstream

kibanamachine and others added 2 commits February 15, 2022 04:31
Changing the default of `rulesListDatagrid` to `true` so that we can play around with this on Cloud.
@gmmorris gmmorris changed the title [RAM] Use EuiDataGrid for the alerts list [RAM] Use EuiDataGrid for the rules list Feb 15, 2022
@kibana-ci
Copy link
Collaborator

kibana-ci commented Feb 15, 2022

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] Default CI Group #10 / Actions and Triggers app Home page Loads the app Alerts tab navigates to an alert details page
  • [job] [logs] Default CI Group #10 / Actions and Triggers app Home page Loads the app Alerts tab navigates to an alert details page
  • [job] [logs] Jest Tests #3 / home renders the documentation link
  • [job] [logs] Default CI Group #13 / ML app anomaly detection alert overview page alert flyout controls can create an anomaly detection alert
  • [job] [logs] Default CI Group #13 / ML app anomaly detection alert overview page alert flyout controls can create an anomaly detection alert

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
triggersActionsUi 322 328 +6

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
triggersActionsUi 234 236 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
triggersActionsUi 678.1KB 711.6KB +33.5KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
triggersActionsUi 54.0KB 55.0KB +1013.0B
Unknown metric groups

API count

id before after diff
triggersActionsUi 246 248 +2

async chunk count

id before after diff
triggersActionsUi 66 68 +2

ESLint disabled in files

id before after diff
apm 15 14 -1
securitySolution 67 66 -1
triggersActionsUi 5 6 +1
uptime 5 4 -1
total -2

ESLint disabled line counts

id before after diff
apm 85 82 -3
triggersActionsUi 154 155 +1
uptime 48 42 -6
total -8

References to deprecated APIs

id before after diff
fleet 14 10 -4
monitoring 40 28 -12
stackAlerts 183 157 -26
upgradeAssistant 8 3 -5
total -47

Total ESLint disabled count

id before after diff
apm 100 96 -4
securitySolution 509 508 -1
triggersActionsUi 159 161 +2
uptime 53 46 -7
total -10

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@ryankeairns
Copy link
Contributor

ryankeairns commented Feb 15, 2022

After chatting with @mdefazio and looking again at the elements depicted in the screenshot above, we had a few more clarifying thoughts for this particular case.

Things you get 'out of the box' with data grid:

  1. The 'Columns' selector UI
  2. Actions attached to cells (including header cells)
  3. Scalability; able to handle many columns and not restricted by width concerns
  4. Advanced, multi-field sorting

Now, for one caveat:

  1. Expandable rows are not supported (previously misspelled as 'now', oops)

As it stands today, if expandable rows are an immediate requirement, then you are faced with using a table and building those custom controls on top/outside. After chatting with @gmmorris about this requirement, we decided to open the discussion on the EUI side to get clarity on future support of this feature within the data grid component. They may be open to They are spec'ing out this feature request and would (likely 😉 ) welcome outside contributors for making it happen.

Update: Security is engaged with EUI and, as I understand it, will be working a PoC from the spec that EUI is writing up!

@chandlerprall
Copy link
Contributor

Issues with too many re-renders mean that moving columns left and right doesn't always work correctly, and column resizing might fail occasionally

Can you expand on this problem? Definitely want to know if it's an issue in the data grid itself, an implementation issue (which we can better document, or maybe resolve entirely through code), or something else going on.

@Zacqary
Copy link
Contributor Author

Zacqary commented Feb 15, 2022

@chandlerprall I still haven't tracked down the exact problem, as we shifted priorities to this PR being a POC instead of a finished implementation. All I know is that you can see the Enabled switches keep doing their initial rendering animation every once in a while whenever you change something in the grid (e.g. sorting, column position) so there's definitely too many renders going on somewhere. I think it's on our end and not the DataGrid's but I haven't been able to figure it out yet.

@Zacqary
Copy link
Contributor Author

Zacqary commented Feb 15, 2022

Also it's not the <AutoSizer> component's fault. I tried removing that entirely and the problem persisted.

@ryankeairns
Copy link
Contributor

Many great implementation considerations (i.e. questions) have been added to the EUI discuss issue. Your team's requirements/use cases would help move things along: elastic/eui#5638 Thanks!

@Zacqary Zacqary closed this Apr 25, 2022
@tylersmalley tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:cloud-deploy Create or update a Cloud deployment Feature:Alerting/RulesManagement Issues related to the Rules Management UX release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants