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

[Monitoring] Fixing issue with EUI table filtering in monitoring UI #27504

Merged

Conversation

chrisronline
Copy link
Contributor

@chrisronline chrisronline commented Dec 19, 2018

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.

@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@chrisronline chrisronline force-pushed the monitoring/eui_filtering_tables branch from cb7b230 to 087afc6 Compare December 20, 2018 18:35
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisronline
Copy link
Contributor Author

chrisronline commented Dec 20, 2018

This PR is blocked until elastic/eui#1318 is resolved. Going to work on getting a PR ready for it

I addressed the core issue requiring the mentioned PR through this commit: 173c79b.

More details in the issue

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisronline
Copy link
Contributor Author

@justinkambic @mattapperson This is now ready for review again!

@elasticmachine
Copy link
Contributor

💔 Build Failed

@chrisronline
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@chrisronline
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@chrisronline
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@chrisronline
Copy link
Contributor Author

retest

Copy link
Contributor

@justinkambic justinkambic left a 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',
Copy link
Contributor

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?

Copy link
Contributor Author

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) {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@chrisronline
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisronline chrisronline added v6.7.0 and removed v6.6.0 labels Jan 9, 2019
@chrisronline chrisronline merged commit 1e9bd95 into elastic:master Jan 9, 2019
chrisronline added a commit to chrisronline/kibana that referenced this pull request Jan 9, 2019
…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
@chrisronline chrisronline deleted the monitoring/eui_filtering_tables branch January 9, 2019 16:02
chrisronline added a commit that referenced this pull request Jan 9, 2019
…27504) (#28365)

* 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
@chrisronline
Copy link
Contributor Author

Backport:

6.x: 9c842ee

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

Successfully merging this pull request may close these issues.

3 participants