-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Monitoring] Fixing issue with EUI table filtering in monitoring UI #27504
[Monitoring] Fixing issue with EUI table filtering in monitoring UI #27504
Conversation
Pinging @elastic/stack-monitoring |
💔 Build Failed |
💚 Build Succeeded |
💔 Build Failed |
cb7b230
to
087afc6
Compare
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
I addressed the core issue requiring the mentioned PR through this commit: 173c79b. More details in the issue |
💔 Build Failed |
💚 Build Succeeded |
@justinkambic @mattapperson This is now ready for review again! |
💔 Build Failed |
retest |
💔 Build Failed |
retest |
💔 Build Failed |
💔 Build Failed |
retest |
💔 Build Failed |
💔 Build Failed |
retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this locally and it seemed to work fine.
Code LGTM, added some comments.
@@ -18,7 +18,7 @@ class ListingUI extends PureComponent { | |||
return [ | |||
{ | |||
name: 'Name', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tangential to this PR, but should these column names be translated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, great catch
@@ -22,6 +22,10 @@ export class EuiMonitoringTable extends React.PureComponent { | |||
search.box['data-test-subj'] = 'monitoringTableToolBar'; | |||
} | |||
|
|||
if (search.box && !search.box.schema) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find any issues related to this but I'm curious about what it's accomplishing. It doesn't conform to the proptypes specified in the docs, so for my own purposes I'm wondering what it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so I'm just allowing any future monitoring tables to specify a schema manually, but if they didn't, set the schema
property to true so the EUI code does it automatically.
💔 Build Failed |
retest |
💚 Build Succeeded |
💚 Build Succeeded |
…lastic#27504) * Ensure the schema is set * Flatten certain table data lists to ensure the default field filtering works * Fix issue with tests * Fix issue with sort key name * Readd localization that was removed
Backport: 6.x: 9c842ee |
This is a follow up PR for #27064 that addresses issues with filtering tables in the new EUI monitoring tables. We basically need to ensure a schema is set for each table for the filtering to work.
To test, please load any/all monitoring UI tables and try and filter using the search bar and ensure the results are as expected.